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

Address JSON test failures #42960

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Address JSON test failures #42960

merged 1 commit into from
Oct 2, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Oct 1, 2020

Updating these tests as they are newly added and resource intensive and they were checked in around the time System.Text.Json tests started timing out on Mono + Windows - #42677.

I assume what is happening here is that the total amount of time it takes to run all the JSON tests has increased beyond the allotted time, which is why the new tests are being disabled, rather than all the unit tests qualified with [Long Running Test] in the various failure logs in the above issue (e.g. this one).


Also making InvalidJsonForTypeShouldFail outerloop to address the failure on net5.0-Windows_NT-Release-x86-CoreCLR_checked-Windows.10.Amd64.Open. Fixes #42817.

@layomia layomia added this to the 6.0.0 milestone Oct 1, 2020
@layomia layomia self-assigned this Oct 1, 2020
@layomia layomia changed the title Address JSON test failures on Mono + Windows Address JSON test failures Oct 1, 2020
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Looks good from an innerloop perspective but I'm concerned about the outerloop quality with this change. Isn't there a way to make this quicker (parallelism) or can't we reduce the number of tests without loosing much or any coverage? cc @ericstj in case he has opinions

@ericstj
Copy link
Member

ericstj commented Oct 1, 2020

@ViktorHofer does outerloop have the same sort of timeouts as innerloop? I thought it didn't. Adding parallelism could decrease execution time of this one test in isolation, but may not shorten the overall test execution if other tests provide enough work to saturate the cores. I don't have enough of a handle over the fanout of our test runner to understand what's best. @ViktorHofer are you aware of guidelines here?

@ViktorHofer
Copy link
Member

I'm not aware of any guidelines here but if my memory serves me right, @stephentoub had to deal with such a large amount of data rows in the past. Stephen do you maybe know to best handle such a large amount of rows?

@layomia
Copy link
Contributor Author

layomia commented Oct 2, 2020

AFAICT we don't have time limits on outer-loop tests, so I'll keep this PR as-is to unblock clean CI per #42677. There likely multiple System.Text.Json tests that could benefit from some optimization, so I'll defer a one-off change for VeryLargeAmountOfEnumDictionaryKeysToSerialize to a follow up PR that addresses tests across the board.

@ViktorHofer
Copy link
Member

There likely multiple System.Text.Json tests that could benefit from some optimization, so I'll defer a one-off change for VeryLargeAmountOfEnumDictionaryKeysToSerialize to a follow up PR that addresses tests across the board.

@layomia do we have a tracking issue for that?

@layomia
Copy link
Contributor Author

layomia commented Oct 3, 2020

@ViktorHofer I'll use #42677 to track this.

@ViktorHofer
Copy link
Member

Thanks

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@layomia layomia deleted the json_tests branch April 26, 2021 17:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long running test: InvalidJsonForTypeShouldFail
3 participants