Fixed Summary Generated Events

[ameetpal]

What?

This PR fixes the shortcomings of TestSummaryGenerated Event :- #3682

Why?

The previously added TestSummaryGenerated had data in the form of a map with keys of type string and values of type io.Reader, however, the stream had already been read by stdout. Additionally, the data was formatted for console output, which was not consistent with what was expected by the handleSummary function of the test. Therefore, I returned the exact summary passed into the function and sent it as an event.

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

[olegbespalov]

LGTM

@codebien and maybe @grafana/k6-core we might consider adding this into upcoming release

[olegbespalov]

Hey @ameetpal. After an internal discussion, we decided to revert to the original changes to reduce it from the scope of the upcoming release #3736 (since they will change). In the next v0.51, we could continue working on that PR. In any case, thank you for your contributions!

[olegbespalov]

Hey @ameetpal

We did release the k6 v0.51. Could you please rebase this PR using the latest master branch? Also, out of curiosity, this version of the changes satisfies your needs, right?

[ameetpal]

Hey @olegbespalov , i have updated the branch, some random tests are failing, but are not related to my changes
Yes these changes are correct

[olegbespalov]

Looks good to me 👍 Thanks for the contribution!

[ameetpal]

@codebien can you please provide your review

[codebien]

As we are changing one of the most important interfaces of the tool, we should add:

1. At least a test in js/summary_test.go testing the returned map
2. Document the new add returned type on the HandleSummary method clarifying the keys and values to expect.

[ameetpal]

@codebien I have added a test for jsSummary.
Also i have documented HandleSummary function but i have not added detailed keys and values as it may change based on a test and also boader key can change in future in runner function which will result in stale comment.
If you still want, i can add comment to current keys that are being returned.

[olegbespalov]

@ameetpal, out of curiosity, do the changes from this PR cover your case from #3732? I mean, have you tested everything assembled? I'm asking since we'd like to be sure that this PR serves the real needs

[codebien]

@ameetpal to exapand a bit @olegbespalov's request. Can you also clarify if this pull request is going to solve #3732 for you?

Can you also explain a bit more about how you intend to use it, please? So, in the case we decide to document this feature, we could use your use case as an example in the documentation. Or, we can have it here just as historical information in case someone in the future needs to investigate more on why we decided to introduce this event.

[ameetpal]

Hey @olegbespalov , @codebien , as of now i will consume this event in the new extension that i am working on. I was thinking of some way to include extension without explicitly importing this in test script but since this is not an option now, we will ask user to import our extension in the test script. I have tested this in local, and that is why i have raised this PR, since i was getting empty data in extension from the changes in my previous PR. Currently this is the only feasible method we find to solve our problem.

[ameetpal]

Also, if we are able to use this successfuly, in future we might flow out other events like TestStarted. For auditing purposes.

[codebien]

Hi @ameetpal,
thanks again for your contribution! 🙇

As you are asserting here, they are mostly the same. So, what is the reason for having them in different variables? Can you explain better it, please?

Ideally, we should try to not change the Runner interface, because it is an abstraction and it is a very central component.

If the current map[string]io.Reader is not the ideal type and we need to change it, we can create a function in js package that accepts this type and returns the type you need. Or, probably, even better to do it directly where we are generating the event so in cmd package.

[ameetpal]

@codebien
Both are same in test as i want to get the data supplied to handleSummary function in test script file.

map[string]io.Reader, contains stdout related data which can only be read once. Also we can use this read also, but we need to write a converter function that will return the json data from which it was created.

If you want i can look into that approach also

[ameetpal]

@codebien any updates here ?

[codebien]

Hi @ameetpal,
I and @olegbespalov met and synced on this pull request.

The current approach doesn't seem to be ideal, because it creates a dependency with an internal API. The current submitted changes rely upon a map[] structure designed internally to the js package. It mostly prepares the structure to serialize the Summary as a JSON. It is used from --summary-export feature, which is already a deprecated part of k6 that we might change in the future.

Instead, a more solid option could be to export lib.Summary type as Data field for the event emitted. In that way, you can create directly in your extension the best structure for your use case. lib.Summary is the type currently passed through HandleSummary and it is already under a shared package (lib).

k6/cmd/run.go

Line 195 in f57dc2f

summaryResult, hsErr := test.initRunner.HandleSummary(globalCtx, &lib.Summary{

Does this proposal still solve your problem?