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

Refactor offset tests #12191

Merged
merged 6 commits into from
Feb 5, 2025
Merged

Refactor offset tests #12191

merged 6 commits into from
Feb 5, 2025

Conversation

AdRiley
Copy link
Member

@AdRiley AdRiley commented Jan 30, 2025

Pull Request Description

  • Change the offset tests to expect table results rather than individual columns results.
  • This is particularly important when we need to test the results are correct but in a different order
  • Add a flag to Table.should_equal to ignore order

Important Notes

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,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@AdRiley AdRiley added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2025
@AdRiley AdRiley force-pushed the wip/adr/refactor-offset-tests branch from 26547f3 to 70c76b6 Compare February 4, 2025 13:00
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

I think it's a good idea to introduce ignore_order argument to Table equality check.

But if we are introducing it, we need to make sure it works for various cases (as noted in the comments, sorting by first column is not sufficient). I'd also suggest adding the "(sorted)" suffix if we are indeed now displaying sorted tables in Actual/Expected OR reverting to displaying the original tables there and noting that the comparison was done ignoring ordering.

I think we can make test failure messages nicer if we borrow from the should_equal_ignoring_order method:

  • The collection X did not contain E.
  • The collection contained an element E that was not expected.
    Could become:
    The table did not contain expected row R / The table contained an unexpected row R and then the Actual/Expected tables can be displayed.

But I think current level of reporting is also good enough - let's just make it precise and either say we are displaying sorted tables or display original ones.

@AdRiley AdRiley added the CI: Ready to merge This PR is eligible for automatic merge label Feb 5, 2025
@mergify mergify bot merged commit 7e651ea into develop Feb 5, 2025
42 of 45 checks passed
@mergify mergify bot deleted the wip/adr/refactor-offset-tests branch February 5, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants