Skip to content

ci: move tests than can be run in parallel now#3038

Merged
georglauterbach merged 4 commits intomasterfrom
ci/move-tests
Jan 30, 2023
Merged

ci: move tests than can be run in parallel now#3038
georglauterbach merged 4 commits intomasterfrom
ci/move-tests

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

Description

Move & renaming of tests that can now run in parallel. I tried to group tests a bit, and I tried to evenly balance the test across the sets. All sets should now take more or less the same amount of time to complete.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach georglauterbach added area/ci kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 29, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Jan 29, 2023
@georglauterbach georglauterbach self-assigned this Jan 29, 2023
@georglauterbach georglauterbach changed the title Ci/move tests ci: move tests than can be run in parallel now Jan 29, 2023
@georglauterbach
Copy link
Copy Markdown
Member Author

tls_ciperlists.bats still takes 6 minutes alone to complete 🤣

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Jan 29, 2023

tls_ciperlists.bats still takes 6 minutes alone to complete

It should average around 5 minutes (300 sec) in my experience, at least on my 4 core/thread i5-6500 😅

Considerations for reducing the test time further

I have tried a variety of things to speed it up, and noticed that single tests could be sped up by 50%, but once running in parallel (testssl.sh mode, not parallel bats which I think I observed before as slowing it down even further), the difference is much less.

Changing to a different base image for testssl showed about 40 seconds or more improvement for me, although I've also been observing a smaller improvement once the base image upgrades to a newer version of Alpine, I'm not 100% sure but it appears to be due to musl vs glibc for the most part.

We can potentially look at splitting the test case up into multiple files grouped in a folder instead (with the methods extracted out into a helper) for bats to run in parallel (each test case runs testssl.sh in parallel on 7 ports though). Alternatively splitting the testssl.sh parallel tests from 7 at once into to two separate runs of 3 and 4 might be another option. I've observed 10-15 seconds for a port to be tested locally, vs around 40-ish for all 7 in parallel, so it's hard to say they'd be much improvement 😅

For CI at least, we can always leverage more runners to spread out those test cases 🤔

Another option is to only run those tests only when relevant files would affect it for PRs, while master branch could run on each merge and raise an issue if the test failed.


Oh, well currently test sets are looking like they'll average around 4 minutes anyway? So it's not that big of a deal, better to reduce the arm build, which I've been looking into and will suss out a PR for this week probably.

Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Are we making snake_case for file names a convention? I noticed we were using kebab-case before.

Thanks for tackling this :)

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Jan 29, 2023

Are we making snake_case for file names a convention? I noticed we were using kebab-case before.

I'd go for snake case, indeed :)


About the current state of the single 6min test: I actually don't mind because it's working fine. Just noting that there is room for future optimisations. I'd go for splitting the single file into multiple too :)

And I totally agree that bringing down ARM build time should be a priority :D

@georglauterbach georglauterbach merged commit e6dee0f into master Jan 30, 2023
@georglauterbach georglauterbach deleted the ci/move-tests branch January 30, 2023 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants