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

Use a single GitHub workflow for testsuite #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atoomic
Copy link
Member

@atoomic atoomic commented Sep 17, 2020

Also run a pre require simple and fast test
before triggering the others to abort earlier
on errors.

You can view a sample run from https://github.com/atoomic/mime-base64/actions/runs/259568440

Also run a pre require simple and fast test
before triggering the others to abort earlier
on errors.
@atoomic atoomic requested a review from genio September 17, 2020 15:33
@genio
Copy link
Collaborator

genio commented Sep 17, 2020

My objection to this is that I can't re-run a particular set of problematic tests on one particular OS (glares at MacOS). If they're all in one file, your only option is to re-run EVERY test rather than the offending macos.yml set of tests. They're blocked off by file, unfortunately

@atoomic
Copy link
Member Author

atoomic commented Sep 17, 2020

Using multiple workflow for testing makes each much harder to read the test result... need to open three tabs to read/check the whole output.

Running tests on macOS and windows is hardware consuming and having them in a single file is the only way to make some cross dependencies between the build jobs. Note the ubuntu job which is what we need in most cases when doing development. If that one fail, we should not even consider running the other OS.

In case you really need to focus on a single OS during a development cycle you can still copy the workflow to a tmp file and adjust it to only keep one available, but do not include as part of your final PR.

@genio
Copy link
Collaborator

genio commented Sep 17, 2020

I'm also not a fan of testing as root on linux. We're using a compiler and testing as root when users will not be doing this. This, admittedly, is more of a problem on dists like Alien::libuv where libuv will not compile if attempted as root.

@atoomic
Copy link
Member Author

atoomic commented Sep 17, 2020

Aren't we using a container while testing? As far as I know it's even fine to run rm -rf /

@genio
Copy link
Collaborator

genio commented Sep 17, 2020

This also takes out the ability to test on multiple Perl versions on MacOS.

That Linux test is not really on ubuntu. Ubuntu starts the docker container for Perl-N but that docker container is really running a different OS and that docker container has you doing everything as root, which is why my linux.yml has the separate steps of forcing them to be run as a different user.

@atoomic
Copy link
Member Author

atoomic commented Sep 17, 2020

Yes I can restore testing multiple versions on macOS if it can help moving forward, but knowing how these runs are expensive for GitHub I'm not sure it has a lot of value as we are already testing the different versions on linux.
I would expect if there is a macOS issue it will show up on all versions.

@atoomic
Copy link
Member Author

atoomic commented Sep 17, 2020

The only test running on ~ubuntu is the pre-requirement which is a ubuntu container.

@genio
Copy link
Collaborator

genio commented Sep 17, 2020

I try to be as nice to GH as I can and only run 2 or 3 versions per OS. I have run into many, many situations where passing on 5.32 doesn't equal passing on 5.10 or 5.14, etc.

@atoomic
Copy link
Member Author

atoomic commented Sep 17, 2020

After exchanging with GitHub the cost of macOS and windows containers run are a much more (>x20 more) expensive than a simple linux run... They should soon release a system where users and organizations would be able to monitor their credit on CI.

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.

2 participants