Skip to content

Conversation

@jsoriano
Copy link
Member

Use json diff to compare results of pipeline tests. This avoids failures caused by different format of equal numbers, as was the case in elastic/integrations#2845.

After this change two json documents are equal if their values are, independently of their format.

The biggest UX change is that this changes the diff to something like this:

panw/panos test-panw-panos-globalprotect-sample.log:
{
    "expected": [
        ...skipped 2 array elements...,
        {
            ...skipped 7 object properties...,
            "panw": {
                "panos": {
                    ...skipped 5 object properties...,
                    "sequence_number": 6920071768563516000 => 6920071768563516860,
                    ...skipped 6 object properties...
                }
            },
            ...skipped 3 object properties...
        },
        ...skipped 1 array element...,
        {
            ...skipped 7 object properties...,
            "panw": {
                "panos": {
                    ...skipped 5 object properties...,
                    "sequence_number": 6920071768563516000 => 6920071768563516847,
                    ...skipped 6 object properties...
                }
            },
            ...skipped 3 object properties...
        },
        ...

The library used has more options to customize the diff format.

We could also decide to show the previous diff if this one detects differences.

@jsoriano jsoriano requested a review from a team March 18, 2022 20:58
@jsoriano jsoriano self-assigned this Mar 18, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 18, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-22T19:29:38.912+0000

  • Duration: 24 min 29 sec

Test stats 🧪

Test Results
Failed 0
Passed 594
Skipped 1
Total 595

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Maybe it's worth adding those problematic tests here?

@jsoriano
Copy link
Member Author

Maybe it's worth adding those problematic tests here?

Do you mean to add the problematic packages as test cases here?

@mtojek
Copy link
Contributor

mtojek commented Mar 21, 2022

Yes, the ones that failed in the Integrations repository.

@jsoriano
Copy link
Member Author

Yes, the ones that failed in the Integrations repository.

@mtojek I have added a simple sample package that reproduces the issues found in elastic/integrations#2845. This will allow us to easily add test cases with numbers in documents.

@jsoriano
Copy link
Member Author

Pushed change to remove colored output and avoid issues with output in CI and so on. See this build for example: https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Felastic-package/detail/PR-743/6/tests/.

@mtojek
Copy link
Contributor

mtojek commented Mar 21, 2022

I guess that we should collect some feedback regarding this change as it may affect the user experience.

/cc our frequent users @kaiyan-sheng @andrewkroh @P1llus @ChrsMark:
What do you think about the new format?

@jsoriano
Copy link
Member Author

/test

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

👍 The more concise output makes it easier to spot the differences.

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

+1 on this change to make it easier to compare! Thanks!

Copy link
Member

@P1llus P1llus left a comment

Choose a reason for hiding this comment

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

Looks awesome! :)

@jsoriano
Copy link
Member Author

/test

@jsoriano jsoriano merged commit 2a298f8 into elastic:main Mar 22, 2022
@jsoriano jsoriano deleted the jsondiff branch March 22, 2022 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants