Skip to content

Conversation

palas
Copy link
Contributor

@palas palas commented Mar 25, 2025

Changelog

- description: |
    Parallelised help golden test generation and validation
  type:
  - optimisation

Context

This is a proposal to parallelise help golden test generation. In my computer, it reduces the execution times from 27 seconds to 3.

How to trust this PR

Don't trust it

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@carbolymer
Copy link
Contributor

Nice!

But it does present some issues with race conditions when recreating.

Do you observe isues in nix / hydra? If not, I'd happily approve - otherwise it's a blocker.

We have GHA tests limited to 1 thread, so that shouldn't be an issue there I think.

@palas
Copy link
Contributor Author

palas commented Mar 25, 2025

Nice!

But it does present some issues with race conditions when recreating.

Do you observe isues in nix / hydra? If not, I'd happily approve - otherwise it's a blocker.

We have GHA tests limited to 1 thread, so that shouldn't be an issue there I think.

I haven't seen any issues in CI because of what you say that it is limited to 1 thread. It does make the RECREATE_GOLDEN_FILES=1 crash consistently if you enable multithreading (even though it does create the files correctly)

@carbolymer
Copy link
Contributor

carbolymer commented Mar 25, 2025

But tests in hydra aren't limited to 1 thread, are they? But we're not using RECREATE_GOLDEN_FILES=1 so they should work from what you're saying.

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

I think in the end I'm against the change. Making tests unreliably fail in the local environment will be very confusing for anyone working on the codebase.

@palas
Copy link
Contributor Author

palas commented Mar 25, 2025

But tests in hydra aren't limited to 1 thread, are they? But we're not using RECREATE_GOLDEN_FILES=1 so they should work from what you're saying.

Exactly

I think in the end I'm against the change. Making tests unreliably fail in the local environment will be very confusing for anyone working on the codebase.

100%, I could try to find some simple solution to that if that is the only issue

@palas
Copy link
Contributor Author

palas commented Mar 25, 2025

@carbolymer, turns out fixing the issue with RECREATE_GOLDEN_FILES=1 was really easy: c19b61d 😎

@palas palas requested a review from carbolymer March 25, 2025 19:08
@carbolymer
Copy link
Contributor

@palas Nice catch! If it makes your solution stable in all cases, then I'm approving.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice 👍

Before optimization: 28.315 s
After Optimization: 20.663 s

Squash the commits

unless isWin32 $ do
test_golden_HelpCmds :: IO TestTree
test_golden_HelpCmds =
-- These tests are not run on Windows because the cardano-cli usage
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@palas palas force-pushed the parallelise-helpcmd-golden branch from c19b61d to 8b3bedb Compare March 26, 2025 14:20
@palas palas enabled auto-merge March 26, 2025 14:23
@palas palas added this pull request to the merge queue Mar 26, 2025
github-merge-queue bot pushed a commit that referenced this pull request Mar 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2025
@palas palas added this pull request to the merge queue Mar 26, 2025
github-merge-queue bot pushed a commit that referenced this pull request Mar 26, 2025
@palas palas removed this pull request from the merge queue due to a manual request Mar 26, 2025
@palas palas added this pull request to the merge queue Mar 26, 2025
Merged via the queue into master with commit a676a7e Mar 26, 2025
26 checks passed
@palas palas deleted the parallelise-helpcmd-golden branch March 26, 2025 17:41
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