Issue #2269: Names starting with underscore must be allowed. Changes …

[elize-vdr]

Names starting with underscore must be allowed.

Issues

This pull request fixes #2269

Description

Changed pattern of StartCharacterExp in Odata.Edm\EdmUtils.cs and added an "underscore" to the pattern.

[elize-vdr]

Not sure if I did something wrong, but this is not getting reviewed?

[mikepizzo]

[mikepizzo]

Changes look good.

Any chance you could add a simple test to validate names starting with the underscore are now allowed, so that we don't inadvertently break this in the future?

[elize-vdr]

Thank you Mike. Yes, sure, good idea, will add a test.

[elize-vdr]

Hi Mike, unfortunately I cannot get the Unit Tests to run, that was the reason why I did not add a unit test, from the start I was unalbe to get it to run, but since there were no other unit tests on the EdmUtils at that level I thought it will be OK, would have liked to add a test, but after truggling and Googling for 5 hours I gave up. If you have a tip for me or know what I might do to get it running I will appreciate it, else there is no way for me currently to get it working. I upgraded my Visual Studio 2019 to the altest to be sure, but did not help. I have tried upgrading the XUnit runner and packages etc, to no avail. Here is a trace of errors I am getting when I run the "IsQualifiedName_Test" in the "EdmUtilTests" of project Microsoft.Odata.Edm.Tests:

[28/12/2021 2:08:43.655 pm] Interrupt: Enqueueing RunSelectedOperation
[28/12/2021 2:08:43.655 pm] Enqueue operation 'RunSelectedOperation', hashcode:4899446 
[28/12/2021 2:08:43.656 pm] Operation left in the the queue: 1
[28/12/2021 2:08:43.656 pm] 	'RunSelectedOperation', hashcode:4899446
[28/12/2021 2:08:43.656 pm] 
[28/12/2021 2:08:43.656 pm] Operation Dequeue : 'RunSelectedOperation'
[28/12/2021 2:08:43.686 pm] Triggering build for 1 IProjectBasedTestContainers.
[28/12/2021 2:08:43.954 pm] No IBuildableTestContainers were found.
[28/12/2021 2:08:43.955 pm] Completed building containers.
[28/12/2021 2:08:43.955 pm] Start updating 1 containers.
[28/12/2021 2:08:43.955 pm] Updating container C:\WorkGlobal\odata.net\bin\AnyCPU\Debug\Test\net452\Microsoft.OData.Edm.Tests.dll.
[28/12/2021 2:08:44.476 pm] Completed updating containers.
[28/12/2021 2:08:45.598 pm] Some tests from the test run selection will be executed by name.
[28/12/2021 2:08:45.599 pm] ========== Starting test run ==========
[28/12/2021 2:08:45.600 pm] Tests run settings for
C:\WorkGlobal\odata.net\bin\AnyCPU\Debug\Test\net452\Microsoft.OData.Edm.Tests.dll:
  
  
    
  
  
    C:\WorkGlobal\odata.net\sln\TestResults
    C:\WorkGlobal\odata.net\sln\
    False
  

[28/12/2021 2:08:46.023 pm] Logging TestHost Diagnostics in file: C:\Users\Elize.van.der.Riet\AppData\Local\Temp\TestPlatformLogs\24720_12_28_2021_14_08_26\logs.host.21-12-28_14-08-45_72980_5.txt
[28/12/2021 2:08:46.653 pm] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.1 (32-bit Desktop .NET 4.0.30319.42000)
[28/12/2021 2:08:47.677 pm] [xUnit.net 00:00:01.02] Skipping: Microsoft.OData.Edm.Tests (could not find dependent assembly 'Microsoft.OData.Edm, Version=7.9.4')
[28/12/2021 2:08:47.701 pm] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.1 (32-bit Universal Windows)
[28/12/2021 2:08:47.707 pm] No test matches the given testcase filter `FullyQualifiedName=Microsoft.OData.Edm.Tests.EdmUtilTests.IsQualifiedName_Test|FullyQualifiedName=Microsoft.OData.Edm.Tests.EdmUtilTests.IsQualifiedName_Test|FullyQualifiedName=Microsoft.OData.Edm.Tests.EdmUtilTests.IsQualifiedName_Test|FullyQualifiedName=...` in C:\WorkGlobal\odata.net\bin\AnyCPU\Debug\Test\net452\Microsoft.OData.Edm.Tests.dll
[28/12/2021 2:08:47.803 pm] ========== Test run finished: 0 Tests (0 Passed, 0 Failed, 0 Skipped) run in 1.2 sec ==========

[mikepizzo]

Sorry for the hassle trying to get the tests to run. We are in the process of cleaning up the projects to make them easier to build/test locally, but right now the .NET Framework tests require strong name signing of the assemblies, so the only way to get them to run is to disable strong name check on your machine (not recommended for production machines).

There are also .NET Core versions of the tests that you could try, although they have been problematic lately as well, and are part of our clean-up effort.

One option would be to add your test and push it to the repo. The CI would then run the test and report any issues. Certainly not ideal, but if the test is pretty straightforward (as I imagine it would be for this issue) it might provide a way forward.

Thanks for your efforts. As I say, we're working on making this easier, but I'd like to figure out a way to take this contribution in the meantime.

[pull-request-quantifier-deprecated]

This PR has 15 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Extra Small
Size       : +14 -1
Percentile : 6%

Total files changed: 2

Change summary by file extension:
.cs : +14 -1

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[ghost]

All CLA requirements met.

[elize-vdr]

I added a test to the EdmUtil tests, please see if that is satisfactory.

[elize-vdr]

Do you perhaps have an indication of when this change will be next released in the nuget package please?

[corranrogue9]

@elize-vdr sorry that you were having trouble running the tests locally. If you can try applying the commits from this branch: corranrogue9/onboarding2 and walking through sln/OData.E2E.md the tests should start working for you locally.

It looks like your test is working successfully on the build machine, so that is a good indication, but it would also be good to make sure that it is working as expected locally. If you can confirm that, we are trying to take this fix in our next release early next week.

[elize-vdr]

Thank you Mike, it will be great if it can be in next release. I am giving the above steps a go to get the tests working locally.

[mikepizzo]

Hi @elize-vdr -- we have merged your PR and it will go out in the next release, targeted for early next week.

Thanks again for your contribution!