Skip to content

Use JsonNode for comparing Json strings to fix the flaky test#1

Open
prathyushreddylpr wants to merge 9 commits into
masterfrom
fix-flaky
Open

Use JsonNode for comparing Json strings to fix the flaky test#1
prathyushreddylpr wants to merge 9 commits into
masterfrom
fix-flaky

Conversation

@prathyushreddylpr

@prathyushreddylpr prathyushreddylpr commented Sep 1, 2023

Copy link
Copy Markdown
Owner

Description

Fixed the flaky test testDateHandling inside TimestampsParquetReaderTest class.

Root Cause

The test testDateHandling has been reported as flaky when run with the NonDex tool. The test failed because it is trying to compare two Json strings, but since the JSON library used here is internally using HashMap. The HashMap in Java is implemented in such a way that it does not store the order in which the keys and values are inserted. As a result, when the expected Json string(which is hard-coded) is compared with the actual one, it caused the failure.

Fix

This test is fixed using the Jackson library which contains classes JsonNode and ObjectMapper. These are used to parse two JSON strings to tree models and then compare these trees node by node. So when we use this, the order of the elements in the JSON strings is ignored and only the content is checked.

How this has been tested?

Java: openjdk version "11.0.20.1"
Maven: Apache Maven 3.6.3

  1. Module build - Successful
    Command used -
mvn install -pl infra/common -am -DskipTests
  1. Regular test - Successful
    Command used -
mvn -pl extensions-core/parquet-extensions test -Dtest=org.apache.druid.data.input.parquet.TimestampsParquetReaderTest#testDateHandling
  1. NonDex test - Failed
    Command used -
mvn -pl extensions-core/parquet-extensions edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.apache.druid.data.input.parquet.TimestampsParquetReaderTest#testDateHandling

NonDex test passed after the fix.

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

@prathyushreddylpr prathyushreddylpr changed the title fix the flaky test Use JsonNode for comparing Json strings to fix the flaky test Sep 1, 2023
@lxb007981

Copy link
Copy Markdown

About PR description text: It might be better to add one more sentence at the end of the Root cause section, like, "As a result, ...".

About code changes: None.

@zzjas

zzjas commented Oct 5, 2023

Copy link
Copy Markdown

Please refrain from using screenshots since it's hard to copy.
Also for code in a sentence, please use backtick quote like this.
Please also include the NonDex commands you used to reproduce the failure (and potentially a link to the NonDex repo).

@Rette66

Rette66 commented Oct 6, 2023

Copy link
Copy Markdown

This PR looks good to me with a clear root cause of the problem and how to solve the flaky test.

@prathyushreddylpr

Copy link
Copy Markdown
Owner Author

Please refrain from using screenshots since it's hard to copy. Also for code in a sentence, please use backtick quote like this. Please also include the NonDex commands you used to reproduce the failure (and potentially a link to the NonDex repo).

updated

@prathyushreddylpr

Copy link
Copy Markdown
Owner Author

About PR description text: It might be better to add one more sentence at the end of the Root cause section, like, "As a result, ...".

About code changes: None.

Updated

@zzjas

zzjas commented Oct 6, 2023

Copy link
Copy Markdown

You can proceed to open a real PR. Once you open a real PR, please mark this tentative PR as Opened in your tentative_pr.csv file and also raise a PR to IDoFT marking this as Opened. Thanks!

@zzjas

zzjas commented Oct 17, 2023

Copy link
Copy Markdown

Have you opened a real PR for this? If so, please mark this as "Opened" in your tentative_pr.csv file and also raise a PR to IDoFT marking this as Opened. Thanks!

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.

4 participants