Skip to content

[Core] Add --publish option #2070

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

Merged
merged 62 commits into from
Aug 12, 2020
Merged

[Core] Add --publish option #2070

merged 62 commits into from
Aug 12, 2020

Conversation

aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jul 26, 2020

This pull request introduced --publish - a simplified interface for publishing reports, similar to cucumber/cucumber-ruby#1452

The report is printed to STDERR, because surefire suppresses STDOUT. The banner is only printed when the URL matches the message store URL. The base URL is hard-coded to https://messages.cucumber.io/api/reports, but can be overridden by defining the CUCUMBER_PLUGIN_PUBLISH_URL environment variable.

TODO:

  • Do not append /api/reports to the base url, and let users define the full URL with CUCUMBER_PLUGIN_PUBLISH_URL
  • Print a banner suggesting the use of --publish so users can more easily discover this feature.
  • Introduce a mechanism allowing users to suppress the banner (a config file)
  • Send additional HTTP authentication headers if an environment variable is defined (CUCUMBER_PLUGIN_PUBLISH_TOKEN)

Screenshots:

Screenshot 2020-07-26 at 10 26 57

Screenshot 2020-07-26 at 10 26 22

@coveralls
Copy link

coveralls commented Jul 26, 2020

Coverage Status

Coverage decreased (-0.006%) to 86.177% when pulling e81fff8 on publish-option into 622bb25 on main.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is a work in progress.

I'm not convinced that reusing the messages plugin is the correct solution here. As a result the PluginFactory now has to be aware of the details of the --publish option. It might be better to introduce a new plugin instead.

And for the todo list the publish option should also be available as an system variable and a property in cucumber.properties and junit-platform.properties. To make this possible I would forgo using addPlugin(...) and instead consider adding a publish flag to RuntimeOptions

Also some minor comments below.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 26, 2020

For a bit of future proving. Consider using cucumber.plugin.publish.* as a prefix for all publish related properties. Currently it is not possible to inject properties into a plugin but we may make this possible in the future. The prefix for a generic plugin would be cucumber.plugin.<plugin-name>.

@mpkorstanje
Copy link
Contributor

Looks like IDEA is mismatching the output to the wrong element in the hierarchy. This looks like an IDEA problem though.

image

@@ -153,3 +153,6 @@ cucumber.snippet-type= # underscore or camelcase.
Each property also has an `UPPER_CASE` and `snake_case` variant. For example
`cucumber.ansi-colors.disabled` would also be understood as
`CUCUMBER_ANSI_COLORS_DISABLED` and `cucumber_ansi_colors_disabled`.


TODO: Document --publish and new properties here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Document --publish and new properties here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly this documentation is also duplicated in cucumber-junit-platform-engine.

@@ -93,6 +93,14 @@
*/
String[] plugin() default {};

/**
* Publish report to https://reports.cucumber.io.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be consistent with the documentation for the options with the same name

@@ -93,6 +93,14 @@
*/
String[] plugin() default {};

/**
* Publish report to https://reports.cucumber.io.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be consistent with the documentation for the options with the same name

@mpkorstanje
Copy link
Contributor

Documentation aside I think we've got everything important now. I'd like to get rid of CucumberProperties.create(); but I don't quite see how yet so I'm willing to leave it. It doesn't seem that important yet.

@mpkorstanje mpkorstanje changed the title Publish option [Core] Add --publish option Jul 31, 2020
mpkorstanje and others added 8 commits July 31, 2020 15:02
We are using the `Runtime.Builder` to construct runtimes for various tests.
To reduce the noise in the logs the publish feature should be off unless
explicitly enabled.

The publish plugin feature is enabled for the main, junit and testng runners.
@aslakhellesoy aslakhellesoy marked this pull request as ready for review August 12, 2020 21:43
@aslakhellesoy aslakhellesoy merged commit a741b27 into main Aug 12, 2020
@aslakhellesoy aslakhellesoy deleted the publish-option branch August 12, 2020 22:18
@mpkorstanje
Copy link
Contributor

@aslakhellesoy

we have a several sources of documentation.

  1. The change log
  2. The release notes
  3. The git log/merge request

And when making a new release I often distil the release notes from the git log and merge requests. However currently neither the git log, nor the merge request describe how this feature works in full. This leaves a gap in the documentation making it harder for future contributors to work out how we intended something to work. This will in turn either result in having to answer questions or people failing to contribute.

image

Cucumber JVM is configured to squash merge all commits. So when merging in Cucumber-JVM you'll get the chance to edit the commit message. I usually use this as an opportunity to update the PR to describe the why and copy this into the commit message.

See also How to Write a Git Commit Message

@aslakhellesoy
Copy link
Contributor Author

👍 I'll deal with this tomorrow before the release.

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

Successfully merging this pull request may close these issues.

3 participants