Skip to content

Jk/cumulus 4556 fix unit#4274

Open
Jkovarik wants to merge 8 commits intomasterfrom
jk/CUMULUS-4556-fix-unit
Open

Jk/cumulus 4556 fix unit#4274
Jkovarik wants to merge 8 commits intomasterfrom
jk/CUMULUS-4556-fix-unit

Conversation

@Jkovarik
Copy link
Copy Markdown
Member

Summary: Summary of changes

Addresses CUMULUS-4556: Develop amazing new feature

This PR includes a minor reliability fix in a unit I noted was intermittently failing during my eval work for 4556.

Changes

  • Makes a minor unit test reliability fix to UpdateGranuleStatusToQueued() updates granule status in PostgreSQL and publishes SNS message"

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Ad-hoc testing - Deploy changes and test manually
  • Integration tests

📝 Note:
For most pull requests, please Squash and merge to maintain a clean and readable commit history.

Copy link
Copy Markdown
Contributor

@Nnaga1 Nnaga1 left a comment

Choose a reason for hiding this comment

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

Just had a question/comment 👀

WaitTimeSeconds: 10,
});
const snsMessageBody = JSON.parse(Messages[1].Body);
const publishedMessage = JSON.parse(snsMessageBody.Message);
Copy link
Copy Markdown
Contributor

@Nnaga1 Nnaga1 Feb 23, 2026

Choose a reason for hiding this comment

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

so above you added receiving a create message -> testing/asserting it -> receive an update message -> testing and asserting, and these changes are basically that where instead of 2 (MaxNumberOfMessages: 2,) you check for the one you just received (the create one), if im understanding correctly and that fixes the race condition

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, ordering isn't ensured, so occasionally the test would fail.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

awesome, thanks for confirming 🙌

Copy link
Copy Markdown
Contributor

@Nnaga1 Nnaga1 left a comment

Choose a reason for hiding this comment

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

LGTM 🙌 thanks for fixing this!

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