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

Cabal test int test #796

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Cabal test int test #796

merged 1 commit into from
Mar 16, 2021

Conversation

kquick
Copy link
Member

@kquick kquick commented Jul 24, 2020

Run intTests via a cabal test operation. Mirrors/replaces runtest.sh functionality.

@kquick kquick mentioned this pull request Aug 7, 2020
Closed
@kquick
Copy link
Member Author

kquick commented Aug 27, 2020

Current status: waiting on merge of PR #668 which will fix test issues to allow final validation of the changes here. Merge of PR #668 is delayed due to a bike test regression.

@robdockins
Copy link
Contributor

I think this should be unblocked now with the merge of #840

RyanGlScott
RyanGlScott reviewed Feb 26, 2021
View reviewed changes
.gitignore
@@ -20,3 +20,11 @@ cabal.project.freeze
*~
*#
.#*
intTests/test_intro_examples/*.cnf
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 you determine what files from intTests to put in .gitignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Chatted w/Ryan, this is in reference to #1067. @LisannaAtGalois , we could perhaps optimistically invoke a make clean after each target, ignoring failures (e.g. no clean target), but we should probably also make sure the gitignore us up-to-date with the latest test outputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a look and manually update the .gitignore (if needed). Maybe this could be automatically linted in CI by running all the tests (or their makefiles?) and checking to see if the git tree is dirty.

Invoking make clean at least on test success makes sense to me. Probably better to have failing tests not invoke that to preserve the output for post-mortem analysis.

@lisanna-dettwyler
Copy link
Contributor

lisanna-dettwyler commented Mar 8, 2021 •
edited
Loading

This is now behaving functionally equivalently to the runtests.sh implementation - fixing the timeout to match what's currently used has all the results lining up now. The last thing this PR needs is to make the test details print on 100% success, and to remove runtests.sh.

Follow-up work to smooth out how the integration tests are themselves specified to make it more cohesive with it being a cabal test target and PR-gating would be awesome:

  • Move saw's Main.hs into a library, invoke main function directly from the tests (which should still include all the command-line argument parsing).
  • Output results in a standard test result format, and then actually do something a bit smarter than just grepping that file afterwards. Regression-comparing the results against master and gating PRs on observed test regression is a really nice way to "automate" the upkeep of PR gating criteria compared to manually-curated lists of disabled / "known failing" cases and the continue-on-error sledgehammer.

kquick
kquick commented Mar 12, 2021
View reviewed changes
intTests/IntegrationTest.hs Outdated Show resolved Hide resolved
@lisanna-dettwyler lisanna-dettwyler self-assigned this Mar 15, 2021
@lisanna-dettwyler
Copy link
Contributor

Pending successful CI after merging in master, this should be ready to go in now.

This does intersect a bit with #1111, which I will update after this is merged.

atomb
atomb approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks!

@kquick
intTests/[runtests.sh -> IntegrationTest.hs]
3ccc0ee 
Enables: cabal test integration_tests

Co-authored-by: Kevin Quick <kquick@galois.com>
@lisanna-dettwyler lisanna-dettwyler added PR: keep updated Magic flag for pull requests to ask Mergify to merge head into the PR branch for you PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run labels Mar 16, 2021
@mergify mergify bot merged commit 2609895 into master Mar 16, 2021
@mergify mergify bot deleted the cabal_test_intTest branch March 16, 2021 20:24
lisanna-dettwyler pushed a commit that referenced this pull request Mar 17, 2021
integrate changes from #796
8a1eda7
lisanna-dettwyler pushed a commit that referenced this pull request Mar 17, 2021
integrate changes from #796
6764fcc
@RyanGlScott RyanGlScott mentioned this pull request Mar 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lisanna-dettwyler lisanna-dettwyler lisanna-dettwyler left review comments

@RyanGlScott RyanGlScott RyanGlScott left review comments

@atomb atomb atomb approved these changes

Assignees

@lisanna-dettwyler lisanna-dettwyler

Labels
PR: keep updated Magic flag for pull requests to ask Mergify to merge head into the PR branch for you PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kquick @robdockins @lisanna-dettwyler @atomb @RyanGlScott