Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for providing concept starting and ending to reporter #2451

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

jensakejohansson
Copy link
Contributor

Solution proposal for issue:
#2450

Added functionality to send start & end concept related messages to plugins.

Related PR in gauge-proto repo is created:

getgauge/gauge-proto#30

@jensakejohansson
Copy link
Contributor Author

@sriv Can you please check this PR too. It's related to the other one you accepted a couple of day ago (getgauge/gauge-proto#30).

I think the checks failed because when they were executed the needed gauge-proto updates were not yet merged. I don't see how to re-run the checks now when the gauge-proto PR is merged. Can you please assist?

@sriv
Copy link
Member

sriv commented Jan 19, 2024

Will do in a few hours time (after work). I tried re-triggering, but it failed. I guess the submodule needs to be updated and protos need to be regenerated in gauge repo.

@sriv
Copy link
Member

sriv commented Jan 19, 2024

I had a look, so this pull request needs to include updates to the proto changes. i.e. you need to run

go get -u github.com/getgauge/gauge-proto/go/gauge_messages to include the concept starting and ending messages. this should change the go.mod and go.sum files.

I tried this on my local and go test ./... passed for me. But it wont let me push the changes to this PR. Could you please try updating the deps and check? the workflows should retrigger.

golangci-lint seems to be unhappy with similar reason, so we can check that post these changes.

Signed-off-by: Piotr Nestorow <[email protected]>
@PiotrNestor
Copy link
Contributor

PiotrNestor commented Jan 19, 2024

@sriv

OK. According to your suggestions:

$ git status
On branch reporter-concept-update

  1. $ go get -u github.com/getgauge/gauge-proto/go/gauge_messages
    go: downloading github.com/getgauge/gauge-proto/go/gauge_messages v0.0.0-20240117152029-6ad2c75d5dea
    go: module github.com/golang/protobuf is deprecated: Use the "google.golang.org/protobuf" module instead.

  2. Checked that all tests work OK

  3. git add --all
    git commit -s -m "Update go.mod and go.sum files"
    git push

@sriv
Copy link
Member

sriv commented Jan 20, 2024

https://github.com/getgauge/gauge/pull/2451/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R3

I guess the go version changed to 1.21.5 when you did the dep update and both golangci-lint and codeql jobs fail because of that. Even the netlify build fails stating the same error - https://app.netlify.com/sites/stupefied-euclid-38c3ac/deploys/65aa72222ab68800082b868c#L66

@PiotrNestor
Copy link
Contributor

@sriv
What is the next step then?

@sriv
Copy link
Member

sriv commented Jan 22, 2024

@PiotrNestor try reverting go version to 1.21?

Signed-off-by: Piotr Nestorow <[email protected]>
@PiotrNestor
Copy link
Contributor

@sriv
Pushed go.mod update to revert to go 1.21

@sriv sriv enabled auto-merge (squash) January 22, 2024 14:11
@sriv sriv disabled auto-merge January 22, 2024 14:11
@gaugebot
Copy link

gaugebot bot commented Jan 22, 2024

@jensakejohansson Thank you for contributing to gauge. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@PiotrNestor
Copy link
Contributor

@sriv
The 'html-report' plugin needs an update now in order to complete the FT steps??
This is a strange chain of dependencies ...
Also there are problems to build the 'html-report' plugin because of problems with github.com/getgauge/mflag.test

@sriv
Copy link
Member

sriv commented Jan 23, 2024

Yes, that is strange. I don't believe that any additions to gauge's contract should impact the plugins.

I will look at html-report today, but I am curious if this impacts other report plugins as well. Will look into it today

@PiotrNestor
Copy link
Contributor

@sriv
Regarding the 'html-report' plugin you can have a look at: https://github.com/system-verification/html-report/tree/reporter-concept-update.

I have updated the plugin with the following:

  1. Added a local 'mflag' package that contains the files from 'https://github.com/getgauge/mflag' with some minor dependency fixes. This because 'github.com/docker/docker/pkg/mflag' is not available anymore
  2. Updated gauge_messages
  3. Updated handler.go. It still needs 'missing method mustEmbedUnimplementedReporterServer'

Obviously you can update the 'html-report' plugin in your way. I think it requires much more work ...

@sriv
Copy link
Member

sriv commented Jan 23, 2024

I have made some changes to html-report. It's failing for some other depreciation but I have removed mflag from html-report and instead am using go's flag package.

Will share more later, not at the computer right now.

Copy link
Contributor

Benchmark Results

java_maven_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 35% 179656 0:34.40 0
523c400 33% 182012 0:32.36 0
9b96118 36% 193468 0:31.56 0
6fe3b27 42% 239700 0:31.56 0

java_maven_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 44% 181400 0:32.12 0
523c400 49% 186688 0:22.21 0
9b96118 41% 178280 0:29.67 0
6fe3b27 72% 233816 0:15.47 0

java_simple_serial.csv

Commit CPU Memory Time ExitCode
77e1343 32% 61496 0:20.52 0
523c400 33% 63508 0:20.32 0
9b96118 25% 63568 0:16.12 0
6fe3b27 41% 67744 0:14.74 0

java_gradle_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 5% 122608 0:46.91 0
523c400 5% 125048 0:37.07 0
9b96118 5% 124252 0:39.01 0
6fe3b27 5% 119756 0:44.06 0

java_gradle_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 7% 97300 0:28.57 0
523c400 6% 102056 0:32.84 0
9b96118 6% 120580 0:36.40 0
6fe3b27 10% 109240 0:23.92 0

java_gradle_serial.csv

Commit CPU Memory Time ExitCode
77e1343 6% 94364 0:40.44 0
523c400 6% 121532 0:45.13 0
9b96118 6% 96004 0:42.09 0
6fe3b27 9% 99352 0:33.72 0

java_simple_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 22% 63788 0:28.84 0
523c400 20% 65628 0:21.50 0
9b96118 20% 65668 0:27.20 0
6fe3b27 24% 74332 0:22.50 0

java_simple_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 26% 63556 0:13.94 0
523c400 27% 65596 0:13.36 0
9b96118 24% 63564 0:14.41 0
6fe3b27 29% 68244 0:12.38 0

java_maven_serial.csv

Commit CPU Memory Time ExitCode
77e1343 53% 198284 0:29.23 0
523c400 51% 205508 0:32.16 0
9b96118 40% 201760 0:33.93 0
6fe3b27 75% 223228 0:18.88 0

Notes

  • The results above are generated by running against seed projects in https://github.com/getgauge/gauge-benchmark
  • These results are not persisted, but on merging to master the benchmark will be rerun.
  • These benchmark are run in Github Actions' agents, which are virtualized. Results are not to be taken as actual benchmarks.Rather, these are indicative numbers and make sense for comparison.

See Workflow log for more details.

@sriv sriv merged commit 62b9937 into getgauge:master Jan 24, 2024
30 checks passed
@sriv
Copy link
Member

sriv commented Jan 24, 2024

thank you for the contribution and more importantly being patient with the merge @jensakejohansson @PiotrNestor

the html-report plugin had to be updated to use the latest proto changes, this is mainly because at the time of the active development of the plugins gRPC did not support mustEmbedUnimplemented for forward compatibility. The latest proto generated go stubs now have this in place and these could help the plugins by not crashing for unimplemented rpc.

@PiotrNestor PiotrNestor deleted the reporter-concept-update branch January 24, 2024 09:10
@jensakejohansson
Copy link
Contributor Author

jensakejohansson commented Jan 24, 2024

Nevermind. I see that a release had been done!

@sriv
Thanks for quick and good support! Next question: If we're going to ask Report Portal to try the new feature & update their plugin accordingly we need to make a release. Is that possible to do?

@chadlwilson
Copy link
Contributor

Hey folks, I'm inferring this is not backward compatible with plugins? See #2456 and getgauge/xml-report#61

Seems a bit unexpected for a patch release?

@sriv
Copy link
Member

sriv commented Jan 29, 2024

yeah, it isn't backward compatible because of some updates to the proto generated code. It should be compatible from now on since the proto tooling has made some enhancements.

And my bad on the version bump - I have created 1.6.0 release, and will take down 1.5.7.

btw - I will also be making changes to xml-report and json-report soon.

@jensakejohansson
Copy link
Contributor Author

Thanks @sriv for fixing. Sorry for causing disruptions with our update/PR. We learn as we go....

@PiotrNestor
Copy link
Contributor

@sriv

A new PR regarding this issue: #2461
ConceptEnd must also be sent when a concept step fails

@sriv
Copy link
Member

sriv commented Feb 2, 2024

The new PR is merged and gauge 1.6.1 is now released. Thanks @PiotrNestor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants