Improve Code Coverage on EnumConverter #39561

Jacksondr5 · 2020-07-17T23:12:44Z

Helps address Help get System.Text.Json test coverage to 100% (or close to it) #32341
Adds coverage to branch where WriteWithQuotes goes over the name cache soft limit. code
Adds coverage to branches where a Dictionary key is a numeric enum that is not in the cache and the value is not a valid identifier. code

dnfadmin · 2020-07-17T23:12:57Z

All CLA requirements met.

Jacksondr5 · 2020-07-17T23:32:52Z

I looked at writing a test for the constructer soft check, but I'm not sure if it's desired. Given that the other tests that cover the cache limit serialize up to 2097152 enums, I think we would need to have a hilariously large enum literal to truly test that the constructor code doesn't throw an OutOfMemoryException. Maybe I could do something with reflection, but I wanted to check before I wrote anything.

Looks like there's a bug in how the coverage reporter interprets switch statements

Hovering over the branch icon on L245 shows that 9 of 11 branches are covered. Given that it's 7 PM on a Friday for me, I don't want to make any strong claims involving math, but those numbers don't seem right to me.

Are we able to cover lines that come after ThrowHelper statements? They look unreachable to me.

Jacksondr5 · 2020-07-25T00:19:16Z

I believe the checks were failing due to a build problem last week that looks to be resolved now. I don't think I have permissions to re-run the builds, can someone trigger that for me?

src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs

jozkee · 2020-08-04T22:02:45Z

src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs

+                };
+
+                Dictionary<T, int> dictionary;
+                for (int i = 1; i < 60; i++)


Can you change this to <= 64 to resemble the soft limit regardless of the amount of cached names in warm-up?

jozkee · 2020-08-05T00:31:38Z

src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs

+            E = 1 << 4,
+            F = 1 << 5,
+            G = 1 << 6,
+            H = 1 << 7,


Seems like H is never used by SerilizeDictionaryWhenCacheIsFull, you should remove it here and in the other enums.

jozkee · 2020-08-05T01:00:43Z

Hovering over the branch icon on L245 shows that 9 of 11 branches are covered. Given that it's 7 PM on a Friday for me, I don't want to make any strong claims involving math, but those numbers don't seem right to me.

I agree, the switch expression only contains 9 cases, I don't see how the coverage reports 9/11 branches, I would expect 8/9 since I don't think we can even cover the default case. Consider open an issue in https://github.com/danielpalme/ReportGenerator.

Are we able to cover lines that come after ThrowHelper statements? They look unreachable to me.

That code is unreachable because we throw but we still need to return in all code paths.

Jacksondr5 · 2020-08-10T03:10:21Z

Dug a bit deeper, the coverage branching issue actually seems to be a problem with coverlet.

…roid (dotnet#40027) This reverts commit 60643c36f5d4e349276e5dcce0e7fc3dc1d72f74. To investigate error on android tests. Co-authored-by: thaystg <[email protected]>

* Fix parsing of embedded .clsidmap in comhost * Update tests to use HostModel to create/embed clsidmap

* s/MscorlibBinder/CoreLibBinder/ * s/g_Mscorlib/g_CoreLib/ * Remaining mscorlib->corelib renames * Delete unreachable FNV NGen string

…tnet#40052)

Co-authored-by: Adeel Mujahid <[email protected]> Co-authored-by: Jan Kotas <[email protected]>

…otnet#39182) * [browser][wasm] Initial addition of configuring request options in Browser WebAssembly * Fix key code. Not what was proposed * Add TryGetValue<TValue> and Set<TValue> as per proposal * Add missing obsolete attribute * Address review comments * Update tests to use Options and not obsolete Properties. * Implement IDictionary<string, object?> explicitly * Update tests to use Options and not obsolete Properties. * Add HttpRequestOptions source to the System.Net.Http.Unit.Tests project to fix build. * Address review comments - explicit * Fix build error cannot convert from 'string' to 'System.Net.Http.HttpRequestOptionsKey<object>' * Add tests for HttpRequestOptions * Fix test build * Add special case code for NETFRAMEWORK for API change. * #endif out of place fix * #endif out of place fix

…g to check for a specific error code. Fixes dotnet#39955 (dotnet#40019)

…#40000)

…ace (dotnet#40004) There is no repro for dotnet#38807 at the moment so the related tests have been enabled. Two tests have been disabled due to PNSE (see dotnet#39346). Fixes: dotnet#38807 The test run result: `Tests run: 177, Errors: 0, Failures: 0, Skipped: 0.`

This was already disabled with autotools, but I forgot to update winconfig.h as well.

…edded (dotnet#40014)

* Update using-dotnet-cli.md Update the First Run section for .NET 5. * more renames * typo Co-authored-by: danmosemsft <[email protected]>

…ce operand of GT_RETURN in case when it is a single-register local variable

…s intrinsics (dotnet#40441)

Fix dotnet#39908

* Minimal fix for Issue 620 Added test case: Runtime_620.cs Added lvForceLoadNormalize * Changed conditional to "else if"

…ils (dotnet#40110) * include more details in exception if remote certificate validation fails * fix unit test linking * feedback from review * update exception message

Co-authored-by: Geoffrey Kizer <[email protected]>

append -y option and add ``` around command code

…tnet#40560)

…et#40548) * [wasm][debugger][tests] Fix negative pointer tests * [wasm][debugger][tests] Fix test to correctly check the valuetype local test: `CheckUpdatedValueTypeFieldsOnResume` * [wasm][debugger][tests] Make value checks consistent - In some places we weren't checking for the `description` property - and this hid a bug where sometimes that property wasn't added (eg. for numbers) - Instead, we were working around that by "fixing it up" later - Now, we use the same checks for `Check{Number,String,*}` API, and the `CheckValue/CheckProps` API used with `TNumber` etc. - So, this commit: - fixes the checks, and the tests - and fixes the bug * [wasm][debugger] Add new `id` types, which have associated properties - these are of the form `dotnet:${scheme}:{id-args-object}` - Examples: - `dotnet:valuetype:{ containerId: 4 }` - `dotnet:valuetype:{ num: 2 }` - the `num` field is autogenerated if no id-args are provided. This gets used when valuetypes are expanded. - `this._id_table [id-string]` has associated property objects for every `id` - This might contain, for example, `klass` pointer, and base64 representation of a valuetype * [wasm][debugger] Update valuetype code to use the new `id`s * [wasm][debugger] Simplify array API in `mini-wasm-debugger.c` .. to use a single function to get details of the full array, and individual elements. * [wasm][debugger] library_mono.js: improvements to valuetype code - Allow `_new_id` to update properties for existing objectIds - Extract valuetype id assigment code to a separate function * [wasm][debugger] mini-wasm-debugger.c- extract object id lookup into a function * [wasm][debugger][tests] Rename method param to be self descriptive * [wasm][debugger][tests] Rework cfo test for getters - add some new getters to the test classes - this will become useful in subsequent commits that add support for invoking getters on valuetypes * [wasm][debugger][tests] Improve valuetype locals/method args tests - this also becomes useful in subsequent commits which enable invoking getters on valuetypes * [wasm][debugger] Add support for invoking getters on valuetypes - keep a copy of the value bytes, and the klass pointer - this allows being able to invoke getters on such a valuetype, at a later point - This allows getters like `DateTime.Date`, which has the type `DateTime` * [wasm][debugger] mono.js: fix warnings .. and replace `var` with `let`, or `const`, where appropriate. * [wasm][debugger] mono.js: _split_object_id -> _parse_object_id * [wasm][debugger] Streamline accessing exported debugger.c functions .. especially the ones that return data in `MONO.var_info`. To use: 1. `this._register_c_var_fn ('mono_wasm_get_object_properties', 'bool', [ 'number', 'bool' ]);` 2. Now, this function can be called as `this.mono_wasm_get_object_properties_info (.. )` - returns `res` which has the contents of `MONO.var_info`, after running `_fixup_name_value_objects` on it. * [wasm][debugger] Return errors from debugger.c's details functions - functions like those for getting object/vt properties, can fail, for example, if the objectId is invalid. - We now return that bool result, and that gets surfaced to the caller - This will also help to differentiate the case where the result of such a function was a failure vs just an empty result * [wasm][debugger] Small checks on inputs, and some negative tests - These tests don't actually depend on the error message, and we don't have another to way to differentiate why a command might have failed with an exception. So, right now, they will pass as long as the commands fail as expected. - Future TODO: return `error`, instead of exception details for issues in `mono.js`, like incorrect input, invalid id etc, and update these tests accordingly. * Update src/mono/mono/mini/mini-wasm-debugger.c Co-authored-by: Larry Ewing <[email protected]> * Remove description checking from TString Co-authored-by: Larry Ewing <[email protected]>

Jacksondr5 · 2020-08-10T03:13:07Z

Think I messed that one up..... I'll open another

Jacksondr5 requested review from jozkee, layomia and steveharter as code owners July 17, 2020 23:12

Dotnet-GitSync-Bot added the area-System.Text.Json label Jul 17, 2020

layomia added this to the 5.0.0 milestone Jul 31, 2020

layomia assigned jozkee Jul 31, 2020

jozkee reviewed Aug 5, 2020

View reviewed changes

jaredpar mentioned this pull request Aug 5, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

danmoseley and others added 17 commits August 9, 2020 23:10

Update visualstudio.md (dotnet#40047)

7627897

Manually merging driver.c from mono/mono#20159 (dotnet#40031)

3af8530

[debugger] Revert Removing unhandled_exception which was used for and…

9412a40

…roid (dotnet#40027) This reverts commit 60643c36f5d4e349276e5dcce0e7fc3dc1d72f74. To investigate error on android tests. Co-authored-by: thaystg <[email protected]>

Disable based on dotnet#40034 (dotnet#40035)

5c66d91

Fix parsing of embedded .clsidmap in comhost (dotnet#40022)

78750fc

* Fix parsing of embedded .clsidmap in comhost * Update tests to use HostModel to create/embed clsidmap

Cleanup mscorlib references under src\coreclr\src (dotnet#39994)

a7a55c6

* s/MscorlibBinder/CoreLibBinder/ * s/g_Mscorlib/g_CoreLib/ * Remaining mscorlib->corelib renames * Delete unreachable FNV NGen string

[interp] Add null checks to STIND opcodes. (dotnet#40041)

876448c

Negate find_object GetConservativeGC logic. (dotnet#39905)

553f9d5

Fix creating RegionInfo object using names with different casing. (do…

a1dd56d

…tnet#40052)

Add Unsafe.IsNullRef and Unsafe.NullRef (dotnet#40008)

86088b2

Co-authored-by: Adeel Mujahid <[email protected]> Co-authored-by: Jan Kotas <[email protected]>

Assert that we don't get the value we don't want instead of attemptin…

6ec162e

…g to check for a specific error code. Fixes dotnet#39955 (dotnet#40019)

Update threadpool callback error messages to fix dotnet#38803 (dotnet…

720ba1b

…#40000)

Disabled outerloop test. (dotnet#40045)

cfb3fab

[mono] Disable cfgdir config on Windows netcore builds (dotnet#40005)

1ea80e9

This was already disabled with autotools, but I forgot to update winconfig.h as well.

Redirect DllImport of hostpolicy to the main executable when it's emb…

7be4343

…edded (dotnet#40014)

AustinWise and others added 13 commits August 9, 2020 23:11

Update using-dotnet-cli.md (dotnet#40504)

ed84ccd

* Update using-dotnet-cli.md Update the First Run section for .NET 5. * more renames * typo Co-authored-by: danmosemsft <[email protected]>

When returning a SIMD value use v0 register as the candidate for sour…

59e4057

…ce operand of GT_RETURN in case when it is a single-register local variable

[Arm64] Treat methods of non-generic Vector64 and Vector128 classes a…

f420714

…s intrinsics (dotnet#40441)

Disable EOL Target Frameworks check in newer SDKs (dotnet#40552)

7869edf

Reenable the test. (dotnet#40515)

046a433

Superpmi: adjust relocations (dotnet#40563)

e23dc14

Fix dotnet#39908

Minimal fix for Issue 620 (dotnet#40535)

ed062dd

* Minimal fix for Issue 620 Added test case: Runtime_620.cs Added lvForceLoadNormalize * Changed conditional to "else if"

include more details in exception if remote certificate validation fa…

565c790

…ils (dotnet#40110) * include more details in exception if remote certificate validation fails * fix unit test linking * feedback from review * update exception message

fix flaky test (dotnet#40565)

7220811

Co-authored-by: Geoffrey Kizer <[email protected]>

Update linux-requirements.md (dotnet#40576)

c1db347

append -y option and add ``` around command code

Update Microsoft.CodeAnalysis.NetAnalyzers to 3.3.0-beta3.20407.4 (do…

66fa808

…tnet#40560)

implement PR feedback

4c03aa1

Jacksondr5 requested review from alexischr, BrzVlad, CoffeeFlux, EgorBo, imhameed, lambdageek, marek-safar, naricc, SamMonoRT, thaystg and vargaz as code owners August 10, 2020 03:12

Jacksondr5 requested a review from a team August 10, 2020 03:12

Jacksondr5 closed this Aug 10, 2020

Jacksondr5 deleted the SystemTextJson-Enum-Test-Coverage branch August 10, 2020 03:18

Jacksondr5 mentioned this pull request Aug 10, 2020

add tests to increase enum serialization code coverage #40601

Merged

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Code Coverage on EnumConverter #39561

Improve Code Coverage on EnumConverter #39561

Jacksondr5 commented Jul 17, 2020

dnfadmin commented Jul 17, 2020 •

edited

Loading

Jacksondr5 commented Jul 17, 2020

Jacksondr5 commented Jul 25, 2020

jozkee Aug 4, 2020

jozkee Aug 5, 2020

jozkee commented Aug 5, 2020

Jacksondr5 commented Aug 10, 2020

Jacksondr5 commented Aug 10, 2020

Improve Code Coverage on EnumConverter #39561

Improve Code Coverage on EnumConverter #39561

Conversation

Jacksondr5 commented Jul 17, 2020

dnfadmin commented Jul 17, 2020 • edited Loading

Jacksondr5 commented Jul 17, 2020

Jacksondr5 commented Jul 25, 2020

jozkee Aug 4, 2020

Choose a reason for hiding this comment

jozkee Aug 5, 2020

Choose a reason for hiding this comment

jozkee commented Aug 5, 2020

Jacksondr5 commented Aug 10, 2020

Jacksondr5 commented Aug 10, 2020

dnfadmin commented Jul 17, 2020 •

edited

Loading