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

CI: Tweaks for backend tests #9001

Merged
merged 4 commits into from
Feb 12, 2024
Merged

CI: Tweaks for backend tests #9001

merged 4 commits into from
Feb 12, 2024

Conversation

mwu-tow
Copy link
Contributor

@mwu-tow mwu-tow commented Feb 7, 2024

Pull Request Description

  • Use glob pattern to discover stdlib tests (rather than a hardcoded list).
  • Don't fail CI check immediately after failing Scala test.
  • Remove meta test suite tests.

Important Notes

The meta test suite tests are removed following the discussion with @radeusgd. In short, these were failing anyway and were supposed to be rewritten (probably using a different technology, like JUnit). The current code will be a useful reference but it doesn't have to be kept on a repository head. The relevant information and references shall be added to the task.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

/// Get paths of directories containing standard library test project.
pub fn discover_standard_library_tests(repo_root: &generated::RepoRoot) -> Result<Vec<PathBuf>> {
// The glob pattern will discover paths like "H:\NBO\enso\test\AWS_Tests\package.yaml".
let glob_pattern = "**/*_Tests/package.yaml";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let glob_pattern = "**/*_Tests/package.yaml";
let glob_pattern = "test/*_Tests/package.yaml";

What about the above?

I'm not sure if we need to or even want to search in other directories.

Copy link
Member

Choose a reason for hiding this comment

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

There are plans to move tests to be next to each library being tested (i.e. something like test folder next to src of each Enso library), so this may need an update then as well.

Copy link
Contributor Author

@mwu-tow mwu-tow Feb 9, 2024

Choose a reason for hiding this comment

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

What about the above?

I'm not sure if we need to or even want to search in other directories.

We attach the glob pattern to the repo_root.test which already paths to the test directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are plans to move tests to be next to each library being tested (i.e. something like test folder next to src of each Enso library), so this may need an update then as well.

Yes. This should not be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

What about the above?
I'm not sure if we need to or even want to search in other directories.

We attach the glob pattern to the repo_root.test which already paths to the test directory.

In that case is the ** even needed?

Unless I'm wrong in this context, usually ** in a glob means multi-level directory traversal and I don't see why that would be needed - in our case we don't need nesting, all tests should be the immediate children of the test dir and looking 'deeper' seems like asking for trouble (if some test project contains an inner project for some reason).

That is a very minor nitpick though, as currently either way works 100% fine and we can always tweak it if needed. So feel free to ignore this suggestion.

…t-tweaks

# Conflicts:
#	test/Meta_Test_Suite_Tests/data/asserts_outside_of_tests/Main.enso
#	test/Meta_Test_Suite_Tests/data/fail_report/Main.enso
#	test/Meta_Test_Suite_Tests/data/with_clue_1/Main.enso
#	test/Meta_Test_Suite_Tests/data/with_clue_2/Main.enso
#	test/Meta_Test_Suite_Tests/src/Main.enso
@mwu-tow mwu-tow self-assigned this Feb 9, 2024
@mwu-tow mwu-tow added -ci -build-script Category: build script CI: No changelog needed Do not require a changelog entry for this PR. labels Feb 9, 2024
@mwu-tow mwu-tow marked this pull request as ready for review February 9, 2024 14:54
@mwu-tow mwu-tow changed the title CI: testing tweaks CI: Tweaks for backend tests Feb 9, 2024
@radeusgd
Copy link
Member

I added the follow-up ticket: #9018

@mwu-tow mwu-tow added the CI: Ready to merge This PR is eligible for automatic merge label Feb 12, 2024
@mergify mergify bot merged commit 357f2aa into develop Feb 12, 2024
27 of 32 checks passed
@mergify mergify bot deleted the wip/mwu/stdlib-test-tweaks branch February 12, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-build-script Category: build script -ci CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants