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 flaky test in 91_flat_object_null_value.yml #15545

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented Aug 31, 2024

Description

This test assumed that the order of returned hits will match the order of insertion. That's not generally true, especially if there was a flush partway through, so documents end up in different segments.

This fixes it by explicitly sorting the returned documents to guarantee that they come back in the correct order.

Also, we were getting a NPE when trying to output the failure message because the expected value was intentionally null. I fixed that too.

Related Issues

N/A (I don't believe we have a flaky-test issue for this one. Thanks @mch2 for bringing it to my attention!)

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

❌ Gradle check result for 8fecdc5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@msfroh msfroh force-pushed the fix_flaky_91_flat_object_null_value.yml branch from 8fecdc5 to 99ff88d Compare September 2, 2024 21:50
Copy link
Contributor

github-actions bot commented Sep 2, 2024

❌ Gradle check result for 99ff88d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Sep 3, 2024

❌ Gradle check result for 99ff88d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Sep 3, 2024

❌ Gradle check result for 99ff88d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@msfroh msfroh force-pushed the fix_flaky_91_flat_object_null_value.yml branch from 99ff88d to 8c187d5 Compare September 3, 2024 19:02
Copy link
Contributor

github-actions bot commented Sep 3, 2024

❌ Gradle check result for 99ff88d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Sep 3, 2024

❌ Gradle check result for 8c187d5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

This test assumed that the order of returned hits will match the order
of insertion. That's not generally true, especially if there was a
flush partway through, so documents end up in different segments.

This fixes it by explicitly sorting the returned documents to
guarantee that they come back in the correct order.

Also, we were getting a NPE when trying to output the failure message
because the expected value was intentionally null. I fixed that too.

Signed-off-by: Michael Froh <froh@amazon.com>
@msfroh msfroh force-pushed the fix_flaky_91_flat_object_null_value.yml branch from 8c187d5 to ba7a34a Compare September 3, 2024 20:55
@msfroh
Copy link
Collaborator Author

msfroh commented Sep 3, 2024

❌ Gradle check result for 8c187d5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Oops -- This one is me. I added the new order field, but there was a test checking that the mapping only contained 1 field.

Copy link
Contributor

github-actions bot commented Sep 3, 2024

❕ Gradle check result for ba7a34a: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=snapshot.status/10_basic/Get missing snapshot status throws an exception}

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.83%. Comparing base (3fc0139) to head (ba7a34a).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #15545      +/-   ##
============================================
- Coverage     71.97%   71.83%   -0.15%     
+ Complexity    64067    63977      -90     
============================================
  Files          5259     5259              
  Lines        299431   299431              
  Branches      43270    43270              
============================================
- Hits         215527   215105     -422     
- Misses        66217    66672     +455     
+ Partials      17687    17654      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msfroh msfroh merged commit eb1cbb8 into opensearch-project:main Sep 3, 2024
34 checks passed
@msfroh msfroh added the backport 2.x Backport to 2.x branch label Sep 3, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 3, 2024
This test assumed that the order of returned hits will match the order
of insertion. That's not generally true, especially if there was a
flush partway through, so documents end up in different segments.

This fixes it by explicitly sorting the returned documents to
guarantee that they come back in the correct order.

Also, we were getting a NPE when trying to output the failure message
because the expected value was intentionally null. I fixed that too.

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit eb1cbb8)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mch2 pushed a commit that referenced this pull request Sep 4, 2024
This test assumed that the order of returned hits will match the order
of insertion. That's not generally true, especially if there was a
flush partway through, so documents end up in different segments.

This fixes it by explicitly sorting the returned documents to
guarantee that they come back in the correct order.

Also, we were getting a NPE when trying to output the failure message
because the expected value was intentionally null. I fixed that too.


(cherry picked from commit eb1cbb8)

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
…5545)

This test assumed that the order of returned hits will match the order
of insertion. That's not generally true, especially if there was a
flush partway through, so documents end up in different segments.

This fixes it by explicitly sorting the returned documents to
guarantee that they come back in the correct order.

Also, we were getting a NPE when trying to output the failure message
because the expected value was intentionally null. I fixed that too.

Signed-off-by: Michael Froh <froh@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants