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

ci: Execute stryker:dry-run on branches #302

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Conversation

karfau
Copy link
Member

@karfau karfau commented Aug 25, 2021

Dry-run works by passing a glob that matches no file.
Additionally we reduce the reporters since there is nothing to report anyways.
I decided to go for a CLI approach instead of a separate config file,
since there is no way to extend config files, and I would like to avoid that we need to keep the two files in sync.

stryker-mutator/stryker-js#3088

@karfau karfau added the infrastructure CI, Dashboards, npm, ... label Aug 25, 2021
@karfau karfau requested a review from brodybits August 25, 2021 06:33
@karfau
Copy link
Member Author

karfau commented Aug 25, 2021

@brodybits I'm thinking about actually splitting the github actions files further, since I think it's actually not required to run npm run lint on every tested node version.
And I want to move the execution of stryker on branches into the stryker related github workflow file.

@karfau karfau marked this pull request as draft August 25, 2021 06:58
@karfau karfau marked this pull request as ready for review August 25, 2021 19:11
@brodybits
Copy link
Member

As a nit, I would favor splitting up test vs lint in a separate PR ... and moving lint into its own YAML file.

LGTM otherwise. I would also favor merging this before we merge any other updates.

@karfau
Copy link
Member Author

karfau commented Aug 26, 2021

Please let me know what you think is the benefit of having the linting in a separate file?
Did you see how the checks look like now? (Already when splitting it into separate jobs it lists 6 checks in the current state: 1xtest for each node version, 1xlint, 1xstryker)

@karfau
Copy link
Member Author

karfau commented Aug 26, 2021

Created #304 as draft to further discuss that aspect. But this one is ready now

brodybits
brodybits previously approved these changes Aug 26, 2021
Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

LGTM thanks!!

Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

nit: I was thinking it would be nice for this picture to show the Stryker check as a dry run:

Screen Shot 2021-08-26 at 9 40 35 AM

@karfau
Copy link
Member Author

karfau commented Aug 26, 2021

@brodybits It's slightly more redundant in the github action, but maybe that's worth it, check 1185c1d and judge yourself.

Feel free to land it if you are fine with it.

Update: I just realized that it now displays the master run as skipped on branches, not sure that's helping:
image

@brodybits brodybits self-requested a review August 26, 2021 20:19
@brodybits brodybits dismissed their stale review August 26, 2021 20:20

I think this still needs some further discussion now

@brodybits brodybits marked this pull request as draft August 26, 2021 20:20
@brodybits
Copy link
Member

brodybits commented Aug 26, 2021

Update: I just realized that it now displays the master run as skipped on branches [...]

Correct, but I think better than showing exactly the same thing for Stryker whether or not it is a dry run.

I would like to try something some things on this now ... hang on

@brodybits brodybits changed the title ci: Execute stryker:dryrun on branches ci: Execute stryker:dry-run on branches Aug 26, 2021
@brodybits brodybits marked this pull request as ready for review August 26, 2021 21:34
@brodybits
Copy link
Member

@karfau I have pushed some updates to separate the changes into a separate YAML file, to get rid of the useless "Skipped" job info, and make a few other, minor improvements.

Using separate YAML files makes the CI testing more modular, at the unfortunate cost of copy-pasted config lines. I would love it if we could figure out how to reduce the amount of boilerplate config that has to be copied in GitHub actions, someday.

Please feel free to update as needed or even discard my changes as you feel appropriate. Please let me know if you want me to merge this, or just merge it yourself if you like. Thanks again for looking into this one.

@karfau
Copy link
Member Author

karfau commented Aug 27, 2021

@brodybits I must say that the amount of time we invest into this detail/"nit" is a bit to much for my taste...

My perspective would be that we wanted a solution that has a chance to inform us when "our stryker setup would not work on master", but now we have "two stryker setups" and have to keep that in mind whenever we change something.

We could of course try to reduce the config to maintain by adding something like https://github.blog/changelog/2021-08-25-github-actions-reduce-duplication-with-action-composition into the mix, but imo this should be a separate PR.

My personal preference (regarding yaml file composition, not about dryrun vs dry-run) is 2e04dd7, just for the sake of not having to remember that we have two separate workflow files for stryker.

Not sure how to proceed here.

@brodybits
Copy link
Member

My personal preference (regarding yaml file composition, not about dryrun vs dry-run) is 2e04dd7

Yeah that sounds reasonable to me. I would love to find a way to improve this further someday, but yeah let's get rid of the multiple Stryker setups.

I would also favor keeping the "-" I stuck into the dry-run but am not super attached to it. I think it does read better with the "-" but it does seem to add a little to the clutter.

@karfau
Copy link
Member Author

karfau commented Aug 27, 2021

Will take care of it

karfau and others added 2 commits August 27, 2021 22:42
Dryrun works by passing a glob that matches no file.
Additionally we reduce the reporters since there is nothing to report anyways.
I decided to go for a CLI approach instead of a separate config file,
since there is no way to extend config files, and I would like to avoid that we need to keep the two files in sync.

stryker-mutator/stryker-js#3088

revert updates to .github/workflows/stryker.yml
@karfau
Copy link
Member Author

karfau commented Aug 27, 2021

I'm very happy about the current state.
@brodybits If it also works for you, just land it.

Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Yeah landing ... thanks!

(This will be my last task before the weekend)

@brodybits brodybits merged commit cc097e5 into master Aug 27, 2021
@brodybits brodybits deleted the ci-stryker-dryrun branch August 27, 2021 20:52
@brodybits
Copy link
Member

I cleaned up the commit message a little, hope I didn't butcher it too badly.

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

Successfully merging this pull request may close these issues.

None yet

2 participants