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

Update opened status and add NOD test #967

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Update opened status and add NOD test #967

merged 1 commit into from
Nov 15, 2023

Conversation

rRajivramachandran
Copy link
Contributor

Opened

PR opened for com.alibaba.fastjson.serializer.TestParse.testParse: alibaba/fastjson#4458
Approved tentative PR: rRajivramachandran/fastjson#1

New test

New test found non deterministic:
com.alibaba.json.bvt.issue_2700.Issue2784#test_for_issue

When the test is run by itself it fails.

[INFO] Running com.alibaba.json.bvt.issue_2700.Issue2784
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.889 s <<< FAILURE! - in com.alibaba.json.bvt.issue_2700.Issue2784
[ERROR] com.alibaba.json.bvt.issue_2700.Issue2784.test_for_issue  Time elapsed: 0.531 s  <<< FAILURE!
junit.framework.AssertionFailedError: expected:<2023-11-11T11:38:15.749028> but was:<2023-11-11T11:38:15.749>
at com.alibaba.json.bvt.issue_2700.Issue2784.test_for_issue(Issue2784.java:21)

The test depends on java.time.LocalDateTime. The test will only pass when the system time taken is an exact millisecond value(i.e microsecond and nanoseconds are zero). The pass scenario has been verified using the following patch.

Since the same test passes and fails when run at different times it is non deterministic. It is not dependent on any other test and is NOD by itself.

VM id: [email protected]
Log passing: /home/rajivr3/fastjson2/fastjson/pass_with_zerobeyondmillis.log
Log failing: /home/rajivr3/fastjson2/fastjson/fail_without_zerobeyondmillis.log

Tentative PR(approved): rRajivramachandran/fastjson#2
Real PR: alibaba/fastjson#4460

@darko-marinov
Copy link
Contributor

Why create ParserConfig in @Before rather than just one line before it's needed?

Does the newly added test pass when run with the others, or does it make the test suite fail almost always?

@rRajivramachandran
Copy link
Contributor Author

rRajivramachandran commented Nov 13, 2023

Prof @darko-marinov

  • The newly added test mostly fails when run with the entire module and run alone. The class it is in has only 1 test. The project has only 1 module.

Module level log: /home/rajivr3/fastjson2/fastjson/test_for_issue/logs/module_level_10runs.log

It only passes rarely(when the instant of test time run had 0 value beyond milliseconds). To simulate and verify that this is the pass scenario I used the patch.

  • For the parse config I created a class level private variable and set it up in @Before so that if future tests are added to the class, they need not do this again. However I understand the concern that this will might again bring in some order dependency if the common object is not being cleared after each test. I shall make change to create the ParseConfig in the test itself.

@rRajivramachandran
Copy link
Contributor Author

@darko-marinov
Copy link
Contributor

If this test fails so often, how is it not affecting the project's CI? Are they not using any CI?

@rRajivramachandran
Copy link
Contributor Author

rRajivramachandran commented Nov 14, 2023

They seem to be using github actions. On every push/PR they do a mvn build according to their workflow: https://github.com/alibaba/fastjson/actions/runs/4710868258/workflow
The last commit on this repo from 6 months back fails the CI: https://github.com/alibaba/fastjson/actions/runs/4955812527 . Unfortunately the logs are archived. In the past there have been many failures: https://github.com/alibaba/fastjson/actions

@darko-marinov darko-marinov merged commit 02fa1c2 into TestingResearchIllinois:main Nov 15, 2023
1 check passed
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