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

Fix ReplSpec tests in GH action #1216

Merged
merged 1 commit into from
May 11, 2023
Merged

Fix ReplSpec tests in GH action #1216

merged 1 commit into from
May 11, 2023

Conversation

rsoeldner
Copy link
Member

@rsoeldner rsoeldner commented May 2, 2023

Previously, we noticed failing tests of the ReplSpec which rely on a pseudo-terminal. The major reason is the parallel execution of the test-suite (using cabal tests).

This PR (should) fix the behaviour. Test PR #1215

Copy link
Contributor

@imalsogreg imalsogreg left a comment

Choose a reason for hiding this comment

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

Let's try.

@@ -168,7 +168,7 @@ jobs:
run: cabal build
- name: Test
shell: bash
run: cabal test +RTS -N
run: cabal run tests
Copy link
Member

Choose a reason for hiding this comment

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

alternatively, since we have some time to look at this deeply, perhaps we should take a look at https://hackage.haskell.org/package/hspec-core-2.11.0.1/docs/Test-Hspec-Core-Spec.html#v:sequential and https://hackage.haskell.org/package/hspec-core-2.11.0.1/docs/Test-Hspec-Core-Spec.html#v:parallel when running these tests in the actual PactTests.hs file, rather than relying on CI to do "the right thing". I'll take a stab at this today and if nothing turns up, let's merge as is, but ideally we'd have these be reproducible at the local level via cabal test.

Copy link
Contributor

Choose a reason for hiding this comment

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

How did that analysis turn out, Emily?

Copy link
Member

@emilypi emilypi May 4, 2023

Choose a reason for hiding this comment

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

Hard to say - i can't get anything to trigger locally. I'm tempted to just say "merge this" and then we can re-evaluate later. I don't want to block.

@imalsogreg
Copy link
Contributor

imalsogreg commented May 2, 2023

@rsoeldner Let's also make note of the time it takes to complete the tests on GitHub, making sure that running them serially doesn't slow CI down too much.

edit:
It appears to take ~3 minutes to run the hspec suite both in your PR and on master.

emilypi
emilypi previously approved these changes May 2, 2023
@emilypi emilypi dismissed their stale review May 2, 2023 22:54

CI does not pass

@rsoeldner
Copy link
Member Author

@emilypi , It looks like this branch passes CI,

We run tests against master and the branch of interest, for each PR. This PR runs the failing tests against master https://github.com/kadena-io/pact/actions/runs/4863641863 (If you click at the action report, checkout repository step, you see master being checked out) and then for my PR https://github.com/kadena-io/pact/actions/runs/4863600190.

@rsoeldner rsoeldner merged commit 37586bd into master May 11, 2023
@rsoeldner rsoeldner deleted the rsoeldner/repl-test-gh branch May 11, 2023 07:57
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.

None yet

4 participants