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

Fix sporadic Table_Tests failures caused by reliance on test ordering. #9438

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Mar 14, 2024

It looks like the test "SQLite_Format should allow connecting to SQLite files" was responsible for cleaning up the SQLite test file, as it removed the file and was last in the test suite. Now that tests run out of order, this won't work.

This PR removes the teardown that was deleting the file early.

#9436 #9437

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.

@GregoryTravis GregoryTravis added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 14, 2024
@GregoryTravis GregoryTravis marked this pull request as ready for review March 14, 2024 20:44
@GregoryTravis GregoryTravis mentioned this pull request Mar 14, 2024
5 tasks
@Akirathan
Copy link
Member

I see. So the SQLite_Format should allow connecting to SQLite files test group deletes the database file in its teardown. If other groups run after that, they will fail because the database file got deleted. Good observation. Note that it is not that necessary to delete that file at the end of testing, as it is deleted at the beginning of testing if necessary.

@@ -539,7 +539,7 @@ add_specs suite_builder setup =
t2.should_fail_with Ambiguous_Column_Rename
err = t2.catch
err.column_name . should_equal "beta"
err.new_names . should_equal ["FirstColumn", "DifferentName!"]
err.new_names.sort . should_equal ["DifferentName!", "FirstColumn"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test probably relied on Map iteration order. (It failed when I changed the iteration order to reproduce the failures.)

@@ -587,7 +587,7 @@ add_specs suite_builder setup =
group_builder.specify "should correctly handle problems: duplicate names" <|
map = ["Test", "Test", "Test", "Test"]
action = data.table.rename_columns map on_problems=_
tester = expect_column_names ["Test 1", "Test 2", "Test 3", "Test"]
tester = expect_column_names ["Test 1", "Test 2", "Test 3", "Test"] ignore_order=True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test probably relied on Map iteration order. (It failed when I changed the iteration order to reproduce the failures.)

@@ -394,6 +394,9 @@ type File_Connection

add_specs suite_builder =
in_file_prefix = "[SQLite File] "
# TODO: Add a suite-level teardown to delete this file at the end.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is removed (if it exists) at the start of the test so it's okay if we don't clean it up for now.

@@ -442,7 +442,7 @@ add_specs suite_builder =
group_builder.specify "should connect to a db file" <|
connection = Data.read data.file
tables = connection.tables
tables.row_count . should_not_equal 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming here that if this test ever ran first, this line would have failed.

@GregoryTravis GregoryTravis merged commit bb080b5 into develop Mar 15, 2024
40 of 41 checks passed
@GregoryTravis GregoryTravis deleted the wip/gmt/9216-macos-tt branch March 15, 2024 14:13
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants