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

File based cli tests using trycmd #2205

Merged
merged 5 commits into from
Dec 5, 2023
Merged

Conversation

Skgland
Copy link
Contributor

@Skgland Skgland commented Dec 4, 2023

Inspired by the work in #2191 for load_module_test tests.
This changes the run_top_level_test_{with,no}_args test to use trycmd to enumerate and run tests.

  • Having .stderr, .stdin and .stdout files improves the readability comparred to multiline strings with escaping

    • this should make adding new test easier
    • .stderr and .stdout can be auto-generated TRYCMD=dump cargo test -- cli_tests
      • will be placed in dump/ for checking and will then need to be moved next to .toml
    • .stderr and .stdout can be updated with TRYCMD=overwrite cargo test -- cli_tests
      • existing .stderr and .stdout will be replaced
  • Previously stderr was ignored, now it is checked to be empty (.stderr files are empty).

  • Path normalization might be a problem as it might normalize slashes in prolog syntax should it be mistaken for a path, but disabling normalization makes windows fail due to different line endings. trycmd only has a shared flag for disabling line ending and path normalization 😞

Note: trycmd enumerates the test at runtime, this is different from #2191 where the tests are enumerated at compiletime. As a result they show up as a single cli_tests test in cargo.

inspired by mthom#2191 but for the `run_top_level_test_with_args`s and  `run_top_level_test_no_args instead of the `load_module_test`s
- goals.pl is used by both compound_goal.toml and multiple_goals.toml therefor renamed the later to share a common prefix with goals.pl, to keep them together
- this re-enables line ending and file path normalization
  - the latter might be unwanted, but there is only a shared flag
  - this might result in too loose matching of / and \ in stdout and stderr as prolog syntax may be mistaken for a path
@Skgland
Copy link
Contributor Author

Skgland commented Dec 4, 2023

Oh, this appears to have some overlap with #2191 as that does not just do load_module_test, as I thought, but also all run_top_level_no_args tests and some run_top_level_with_args test.

@infogulch
Copy link
Contributor

infogulch commented Dec 4, 2023

Nice!

I liked that 2191 enumerated the tests within the standard rust test harness, but I like that it actually works even more. 🤷

It would be nice to be able to interact with the machine as a library replacing stdin|out with programmatically managed byte streams, but it seems that this is either not possible or at least it's not known how to do it today. Maybe I'll open a separate issue to discuss the merits of this goal and track progress.


Yes I think it would be good to unify the different test suites as far as possible, converting most tests to one format. IMO, ideally all the prolog tests would be moved under the /test directory, and the rust test harness would be moved somewhere underneath /src. This is sortof what I was aiming at with 2191, but this design could take it even farther if others agree with the direction.


Maybe the test enumerator could use .fail("tests/scryer/cli/issues/singleton_warning.toml") // wrong line number instead of .skip(...)? See: https://docs.rs/trycmd/latest/trycmd/

@Skgland
Copy link
Contributor Author

Skgland commented Dec 5, 2023

Yes I think it would be good to unify the different test suites as far as possible, converting most tests to one format. IMO, ideally all the prolog tests would be moved under the /test directory, and the rust test harness would be moved somewhere underneath /src. This is sortof what I was aiming at with 2191, but this design could take it even farther if others agree with the direction.

Having integration tests in tests/*.rs or tests/*/main.rs is the default as cargo auto-detects them there by default, so I wouldn't want to change that. But moving the test cases (.toml,.stderr,.stdin,.stdout,.pl) into test/ should be fine.

I don't think any of these make particularly sense to convert to unit tests in the library modules in src/.

Maybe the test enumerator could use .fail("tests/scryer/cli/issues/singleton_warning.toml") // wrong line number instead of .skip(...)? See: https://docs.rs/trycmd/latest/trycmd/

I tried that but that didn't appear to have the desired effect. Instead of expecting a general test case failure it expected a failing exit code, but still expected the output to match, which is not what we need here.

@mthom mthom merged commit 43556f6 into mthom:master Dec 5, 2023
13 checks passed
@triska
Copy link
Contributor

triska commented Dec 5, 2023

@Skgland: singleton_warning.stdin references 'tests/scryer/cli/issues/issue812-singleton-warning.pl', a file that does not exist?

@Skgland
Copy link
Contributor Author

Skgland commented Dec 5, 2023

@Skgland: singleton_warning.stdin references 'tests/scryer/cli/issues/issue812-singleton-warning.pl', a file that does not exist?

tests-pl/issue812-singleton-warning.pl should have been moved to that location, but was deleted instead, not sure how that happened.

Skgland added a commit to Skgland/scryer-prolog that referenced this pull request Dec 6, 2023
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.

4 participants