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

Added minitest-retry gem to retry flapping tests that are able to pass through retries #3044

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

…t from preservation queue rake task when running tests
Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

Just have a few questions. This is looking good!

@@ -125,6 +125,7 @@ group :test do

gem 'json-schema', '~> 3.0.0'
gem 'launchy'
gem 'minitest-retry', require: false
Copy link
Member

Choose a reason for hiding this comment

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

Nice find! Does it add much more time to running our test suites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to say as the time it takes for the tests to run varies quite a lot run to run on CI. It looks like it is taking around as long as it used to without this gem but it would require further testing to know for sure.

collection_and_community_count = Community.count + Collection.count
assert_equal collection_and_community_count,
RedisClient.current.zcard(Rails.application.secrets.preservation_queue_name)
$stdout.stub(:puts, nil) do
Copy link
Member

Choose a reason for hiding this comment

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

That's a tidy workaround for the extra output. What do you think about adding a comment explaining why this block is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some refactoring and added a comment in my most recent commits

Copy link
Member

Choose a reason for hiding this comment

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

Nice refactor!

Comment on lines +15 to +21
AdminUsersIndexTest#test_should_be_able_to_autocomplete_by_name
BatchIngestTest#test_invalid_without_files
DepositThesisTest#test_be_able_to_deposit_and_edit_a_thesis_successfully
DraftControllerTest#test_should_not_be_able_to_update_a_draft_item_when_saving_upload_files_form_that_has_no_file_attachments
DraftControllerTest#test_should_not_be_able_to_update_a_draft_thesis_when_saving_upload_files_form_that_has_no_file_attachments
ItemListFilesTest#test_files_are_alphabetically_sorted_when_depositing_an_item
ThesisListFilesTest#test_files_are_alphabetically_sorted_when_depositing_an_item
Copy link
Member

Choose a reason for hiding this comment

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

Are we tracking these in a ticket to review at some point? This list is longer than I was expecting. I recognize DepositThesisTest#test_be_able_to_deposit_and_edit_a_thesis_successfully

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've added a ticket for this here

Copy link
Member

Choose a reason for hiding this comment

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

I added the failure message in case we don't get back to it before the log becomes inaccessible after 90 days.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you!

Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

LGTM!

@ConnorSheremeta ConnorSheremeta merged commit 38a9688 into master Jan 27, 2023
@ConnorSheremeta ConnorSheremeta deleted the cds/retry_flapping_tests branch January 27, 2023 22:43
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.

2 participants