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

start checking some third party builds for regressions #537

Merged
merged 1 commit into from
May 14, 2022

Conversation

mvdan
Copy link
Member

@mvdan mvdan commented May 13, 2022

(see commit message)

Fixes #240.

@mvdan
Copy link
Member Author

mvdan commented May 13, 2022

Note that this is fixing the issue even though I've only implemented the basic form of what was discussed. This is on purpose; I'd rather not leave an umbrella issue open with vague ideas, because then it could stay open for a long time.

I've also grown less convinced that running the tests will be useful. Some tests will simply break with obfuscation by design, if they rely on position information or reflection. Other tests may be slow or flaky. Overall, I'm not convinced that running them as part of CI will catch significantly more bugs; the majority of bugs and regressions in garble are because garble build fails, not because the resulting binary is misbehaving.

We could always add a mode to the script, like ./scripts/check-third-party.sh --with-tests, which would also run garble test -short ./..., and perhaps we can run that locally from time to time.

Our tests should already be pretty extensive,
and any bug fixes should result in more regression test cases,
but testing against a few diverse and popular third party modules
will help prevent unintended regressions while developing garble.

The list is short for now. More can be added later.
This adds protobuf and wireguard from the original issue,
but not cobra and logrus, as they aren't particularly complex nor add
significant variety on top of protobuf and wireguard.

While here, we remove the job that only runs crlf-test.sh,
as we don't really need a separate job for a tiny script.

Fixes burrowers#240.
@mvdan
Copy link
Member Author

mvdan commented May 13, 2022

the ubuntu-latest 1.18 job goes from ~12m to ~13m, so not too bad.

Copy link
Member

@pagran pagran left a comment

Choose a reason for hiding this comment

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

We've discussed this before, but I'll ask again: How about creating a separate repository that will asynchronously test this way?

.github/workflows/test.yml Show resolved Hide resolved
@mvdan
Copy link
Member Author

mvdan commented May 14, 2022

How about creating a separate repository that will asynchronously test this way?

Test asynchronously... how? I see two main options:

  1. CI jobs in this repo trigger CI jobs in the other repo. The other repo has to re-do the work to clone and build garble, and seeing the results from the other repo is also an extra step.

  2. CI jobs in the other repo are triggered on their own, like once a week. This could keep our CI slightly faster, but then we wouldn't notice when a merge creates a regression. The main point of CI is to keep regressions out of master :)

In fact, either one of these can be done via separate jobs/workflows in the same repository, so a separate repo doesn't gain us anything. I'd personally only go for a separate repo if we absolutely had to, if e.g. these third party tests required checking in tons of files into git and we wanted to keep the main repo small. But that isn't the case.

@mvdan mvdan merged commit 201d890 into burrowers:master May 14, 2022
@mvdan mvdan deleted the check-third-party branch May 15, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

select relevant third-party tests to run their tests with garble on CI
3 participants