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.

Sign up for GitHub

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

Jump to bottom

Set NumThreads to 1 in cardano-testnet-test #5758

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Set NumThreads to 1 in cardano-testnet-test #5758

merged 1 commit into from
Apr 11, 2024

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Apr 6, 2024 •
edited
Loading

Description

Make tests single-threaded by setting NumThreads 1. This has the advantage that --list-tests still work.

This replaces #5744 because that PR fails in CI for an as yet unknown reason.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@newhoggy newhoggy force-pushed the newhoggy/num-threads-to-1-in-cardano-testnet-test branch from 40d5e3a to 384878e Compare April 7, 2024 13:09
@newhoggy newhoggy marked this pull request as ready for review April 8, 2024 10:09
@newhoggy newhoggy requested a review from a team as a code owner April 8, 2024 10:09
carbolymer
carbolymer reviewed Apr 8, 2024
View reviewed changes
cardano-testnet/test/cardano-testnet-test/cardano-testnet-test.hs
[ H.ignoreOnWindows "transaction" Cardano.Testnet.Test.SubmitApi.Babbage.Transaction.hprop_transaction
]
]
]

shouldRunInParallel :: IO Bool
shouldRunInParallel = (== Just "1") <$> E.lookupEnv "PARALLEL_TESTNETS"
defaultMainWithIngredientsAndOptions :: [T.Ingredient] -> T.OptionSet -> T.TestTree -> IO ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just copied defaultMainWithIngredients with ability to change OptionSet or is there something else added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct.

carbolymer
carbolymer reviewed Apr 8, 2024
View reviewed changes
cardano-testnet/test/cardano-testnet-test/cardano-testnet-test.hs
@@ -105,4 +108,6 @@ main = do
hSetEncoding stdout utf8
args <- E.getArgs

E.withArgs args $ tests >>= T.defaultMainWithIngredients ingredients
let opts = T.singleOption $ T.NumThreads 1
Copy link
Contributor

@carbolymer carbolymer Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we have an environment variable to override this? It's still useful to me to be able to run tests in parallel locally.

Copy link
Contributor Author

@newhoggy newhoggy Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You should be able to do this:

--test-options '--num-threads 8'

Because all I've done is change the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is:

  defaultValue = NumThreads numCapabilities

Whatever that works out to be.

@newhoggy newhoggy added this pull request to the merge queue Apr 8, 2024
@newhoggy
Copy link
Contributor Author

newhoggy commented Apr 8, 2024

Also see: UnkindPartition/tasty#414 and UnkindPartition/tasty#412

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 8, 2024
@carbolymer carbolymer mentioned this pull request Apr 8, 2024
Open
@carbolymer carbolymer added this pull request to the merge queue Apr 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 9, 2024
@carbolymer carbolymer added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2024
@newhoggy newhoggy added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2024
@carbolymer carbolymer added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 9, 2024
@carbolymer carbolymer added this pull request to the merge queue Apr 10, 2024
@carbolymer
Copy link
Contributor

@newhoggy I'm wondering if the PR can't pass the merge queue because of your changes or if it's because our cardano-testnet flakiness

@newhoggy newhoggy removed this pull request from the merge queue due to a manual request Apr 10, 2024
@newhoggy newhoggy force-pushed the newhoggy/num-threads-to-1-in-cardano-testnet-test branch from 384878e to 73a3cbc Compare April 10, 2024 12:45
@newhoggy newhoggy added this pull request to the merge queue Apr 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 10, 2024
@newhoggy newhoggy added this pull request to the merge queue Apr 11, 2024
Merged via the queue into master with commit a152b00 Apr 11, 2024
22 checks passed
@newhoggy newhoggy deleted the newhoggy/num-threads-to-1-in-cardano-testnet-test branch April 11, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@carbolymer carbolymer carbolymer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @carbolymer @Jimbo4350