feat(test): Changed testing system to allow more granular checks#1876
feat(test): Changed testing system to allow more granular checks#1876Shikhar-Srivastava-16 wants to merge 38 commits into
Conversation
|
Automatically assigned this pull request to @Shikhar-Srivastava-16. If it makes more sense for someone else to own it, feel free to reassign it. |
Code CoverageReport: https://conjure-cp.github.io/conjure-oxide-reports/pr/1876/coverage-code/index.html This PR: 72.91% lines (22689/31121) |
RustDoc CoverageReport: https://conjure-cp.github.io/conjure-oxide-reports/pr/1876/coverage-docs/index.html This PR: 44.29% documented, 2.85% with examples (37/574/1296) |
|
This PR is too big for Vale to parse. |
|
@ozgurakgun this is the PR I was talking about, please take a look and let me known if you think this is a good idea can also cherry-pick the name change if you would rather not wait for this PR |
sorry I don't remember the conversation - can you give me some hints? :) |
Sorry, should have made it clearer - the PR I was talking about in my comment on your issue (#1475). Long story short, it renames the tests-integration directory and I wanted you to be aware so you don't also do it |
|
Got it! In fact I think I'll make a PR that just renames that directory and nothing else. You can rebase this on top of that I guess. I think I'll call it |
|
there are a million changes in the test files, so this will take a while to review properly. but in the meantime I noticed a bunch of |
a small bit of #1876 fyi @Shikhar-Srivastava-16
ozgurakgun
left a comment
There was a problem hiding this comment.
- Remove generated files
- There are conflict markers in some of the files. Search for <<<<
- The golden test files get renamed for now reason. We append the solver name in two places.
assert_no_redundant_expected_filesis removed, we should keep thatsetup_integration_testsuses HashSet which is redundant
@ozgurakgun thank you for pointing this out. I will make all of the other changes, but I am not sure how the golden test files work, could you please let me know what changes I should make other than adding back the Additionally, the most recent commits have some sort of issue with Minion, and I have no idea what is wrong with that, could you please help out with that? |
todo: golden file test is causing problems because it looks by directory and should probably be done individually
As a follow up on this previous message, I have made all necessary changes other than checking for redundant golden files. The problem is that currently, we check for all files with the Each test (which pertains to a single runcase) will find golden files for all other runcases on the same file in its 'parent' directory, and therefore fail. There are a few ways to quickly fix this. The first is to perform, with each test, a check for golden files for all runcases, which leads to massive redundancy. The second is to only check for files of a certain runcase, which may miss some redundant (or irrelevant) files. The third is to do this check in build.rs before enumerating over the runcases. This will fix both the problem at the cost of forcing the user to fix these problems before allowing them to even run tests. @ozgurakgun please let me know what you think. I am leaning towards the third option. |
|
Sorry, I am slow to respond to this one, but I have been focusing on #1893, where I change quite a bit about the integration tester as well. |
|
@ozgurakgun When you next review this PR, please be careful because there are a bunch of changes to the roundtrip tests. This is not due to anything I have changed. I believe it is because I improperly rebased after another PR was merged. Please also keep in mind that I have edited the description to have a few new changes. |
|
Thanks @Shikhar-Srivastava-16 I'll get to this once #1893 lands and that is waiting for Georgii's representation changes to land... I have a feeling @gskorokhod will be busy with graduation this week :) So this might take a while. |
Thank you for letting me know. I do think that this PR might make your life easier for #1893 in particular, as one of the things you're having to do (according to the description) is disabling backends to test one solver at a time. One of the features this PR adds is the ability to just run all tests for a single solver, which might save you some work. Additionally, the main reason I made this PR (and why I've been pushing this change harder than usual) is because I wanted to be able to run tests with this granularity so that I could more easily adapt the SAT code to the new representation system. Ultimately, of course, it is up to, but I thought that this information might be relevant. |
Description
Changed the testing system to generate a different test for each runcase for each essence file
Related issues
resolve #1475 (changed to 1880 and other PRs)
related to #1822 (vaguely)
Key changes
runcaseinto test_config moduleruncaseper testruncasebuild.rsto enumerate over runcases instead of enumerating inintegration_test()AcceptModeto the golden_files. This is because AcceptMode becomes a dependecy of the build script due to the next changeHow to test/review
Use
cargo test integrationUse
cargo test integration sat-direct-- This could not be done before and is a new way of only running the tests that use a specific solver config