All the regex parsing tests in their own branch. by CyrusNajmabadi · Pull Request #25520 · dotnet/roslyn

CyrusNajmabadi · 2018-03-16T06:24:52Z

Followup to #23984. I've pulled out the tests to make that PR easier to review.

CyrusNajmabadi · 2018-03-16T22:24:44Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

jcouv · 2018-04-04T04:26:59Z

src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests.cs

+                Assert.True(regex.GetGroupNames().Where(v => !int.TryParse(v, out _)).OrderBy(v => v).SequenceEqual(
+                    tree.CaptureNamesToSpan.Keys.OrderBy(v => v)));
+            }
+            catch (IndexOutOfRangeException)


Try/catching seems too loose. Can tests that trigger those cases pass a parameter to signal their expectation? #Closed

sure. i can do that. #Closed

Ok. I did this. Tests now need to explicitly state if they expect hte regex constructor to throw. Also, we only wrap creating the .net regex in a try/catch. That way we're only checking for exceptions in it, and we're not accidentally catching exceptions in the roslyn regex system. #Closed

jcouv · 2018-04-04T04:27:57Z

src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests.cs

+            Assert.Null(tree);
+        }
+    }
+}


Maybe I missed it, but do we check round-tripping? #Closed

I don't. But that's a good idea. I will add that to the tests! #Closed

Oh, nevermind. I do that test that. Those are the CheckInvariants and CheckCharacters calls that we make. #Closed

I'm thinking just a ToString() call on RegexTree and comparing with the known string. #Closed

I'd prefer to not create a ToString (yet) that isn't used anywhere but tests. We already enforce hte invariants here through the supported API. #Closed

jcouv · 2018-08-22T20:57:06Z

...ces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_TestGeneration.cs

+    }
+
+    [Collection(nameof(MyCollection))]
+    public partial class CSharpRegexParserTests


CSharpRegexParserTests [](start = 25, length = 22)

From reading the doc on Collection, I expected to find some tests (annotated with [Fact]) in this class.
Is this meant to only be run manually? #Closed

Yup. This was all done manyally for generatin tests. (hence _TestGeneration). #Closed

Ok. It seems useful to keep this file, but please add some documentation.

In reply to: 212121143 [](ancestors = 212121143)

Given that this code already provides a way of regenerating the baselines. What do you think of better representing spans?

In reply to: 212122968 [](ancestors = 212122968,212121143)

jcouv · 2018-08-22T21:02:40Z

<Capture Name=""0"" Span=""[10..42)"" />

Such spans are not very useful (reviewer cannot verify). I would have recommended quoting the text of that span... :-S #Closed

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:166 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv · 2018-08-22T21:13:50Z

<Capture Name=""0"" Span=""[10..42)"" />

To update so many test baselines, Fred has a tool which could be useful (with some adaptation).
https://github.com/333fred/IOperationTestFixer

The basic flow:

run Pilchie's test runner and copy the test errors in a file
run the fixer (it will parse the test errors and collect "Actual" strings with corresponding test names, then it will fix the tests)
The fixer would need to be adapted, as it currently only understands the pattern of tests used for IOperations.

I feel pretty strongly about doing something to improve the readability of these spans, but I'd be ok if it's done separately.

In reply to: 415178848 [](ancestors = 415178848)

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:166 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv · 2018-08-22T21:16:42Z

<Capture Name=""0"" Span=""[10..42)"" />

Another idea would be to fix the test framework so that any test that is added or updated from now on would include more details, but all those initial tests would be left as-is.

In reply to: 415182382 [](ancestors = 415182382,415178848)

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:166 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv · 2018-08-22T21:19:51Z

        Test(@"@""((?<One>abc)\d+)?(?<Two>xyz)(.*)""", @"<Tree>

Is it possible to use digits for capture names? For instance, 0 or 1?
What happens in such case? #Closed

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:105 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv · 2018-08-22T21:21:47Z

public partial class CSharpRegexParserTests

In general, I didn't understand the separation of tests: "Basic", "DotnetNegative" and "Reference". Could you clarify?
If the categorization isn't quite clear, I'd be tempted to merge the three files. #Closed

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:8 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv

Done with review pass (iteration 54).

I think something should be done about validation of spans in captures. We can discuss some options.

I need some overview for "TestGeneration" file, so that I can effectively review it.

CyrusNajmabadi · 2018-08-22T21:48:27Z

Another idea would be to fix the test framework so that any test that is added or updated from now on would include more details, but all those initial tests would be left as-is.

I would be ok with that. :)

CyrusNajmabadi · 2018-08-22T21:49:22Z

Is it possible to use digits for capture names? For instance, 0 or 1?

Yes. You can see that in ReferenceTest123.

CyrusNajmabadi · 2018-08-22T21:50:43Z

In general, I didn't understand the separation of tests: "Basic", "DotnetNegative" and "Reference". Could you clarify?

I can comment.

"basic" is me going through every codepath in the lexer/parser and trying to hit it.
"dotnetnegative" are the existing tests from corefx that all fail.
"reference" is taking all the strings from https://docs.microsoft.com/en-us/dotnet/standard/base-types/regular-expression-language-quick-reference and making tests out of all of them.

CyrusNajmabadi · 2018-08-22T21:52:29Z

I need some overview for "TestGeneration" file, so that I can effectively review it.

This file was used by me for some times when i had to go update like a thousand tests at once. It was easier to basically run the existing tests through a helper funciton that then spat out the new test baselines. I don't really need it anymore and it can be removed. For anything that has spans right now (for example Captures, or Diagnostics), i'm happy to make it include text. In that regard, TestGeneration would likely be useful to respit out all the test baselines again.

jcouv

LGTM Thanks (iteration 58).

CyrusNajmabadi · 2018-08-22T22:10:22Z

Given that this code already provides a way of regenerating the baselines. What do you think of better representing spans?

SGTM. I'll do so tonight. I'll leave the spans in, but i'll also give the text those spans represent. Does that work for you?

jcouv · 2018-08-22T22:13:21Z

but i'll also give the text those spans represent.

Yes, that sounds good. Thanks!

CyrusNajmabadi · 2018-08-22T22:38:11Z

@jcouv I've updated all the baselines (yaay for having a tool to do that) :) Let me know what you think.

jcouv · 2018-08-22T22:47:06Z

<Capture Name=""0"" Span=""[10..42)"" />

Awesome. That's much better!

In reply to: 415183243 [](ancestors = 415183243,415182382,415178848)

Refers to: src/Workspaces/CSharpTest/EmbeddedLanguages/RegularExpressions/CSharpRegexParserTests_ReferenceTests.cs:166 in 7767ce6. [](commit_id = 7767ce6, deletion_comment = False)

jcouv

LGTM (iteration 63).
Thanks a lot for addressing the span issue!

CyrusNajmabadi · 2018-08-22T22:52:30Z

Awesome. That's much better!

Also, a super great reason to keep around the baseline regenerator :D

CyrusNajmabadi · 2018-08-23T03:16:21Z

@jcouv can you merge in?

Merge branch 'regexParsing4' into regexTests

563d85e

CyrusNajmabadi requested a review from a team as a code owner March 16, 2018 06:24

This was referenced Mar 16, 2018

Extract out the VirtualChar portion of the Regex and JSON editor work. #24111

Merged

Add in a regex parser so that the IDE can provide services around regex editing. #23984

Merged

CyrusNajmabadi changed the title ~~All the regex tests in their own PR.~~ All the regex parsing tests in their own branch. Mar 16, 2018

CyrusNajmabadi added 8 commits March 16, 2018 00:47

Merge branch 'regexParsing4' into regexTests

53096d7

Merge branch 'regexParsing4' into regexTests

40f2ff2

Merge branch 'regexParsing4' into regexTests

7242b5b

Update tests.

b7c850e

Move tests.

b8e8f34

Merge branch 'regexParsing4' into regexTests

209615d

Merge branch 'regexParsing4' into regexTests

74ab6bf

Merge branch 'regexParsing4' into regexTests

241e8ba

CyrusNajmabadi added 3 commits March 24, 2018 22:08

Merge branch 'regexParsing4' into regexTests

039cf44

Merge branch 'regexParsing4' into regexTests

f7a0796

Merge branch 'regexParsing4' into regexTests

c4e0ddb

CyrusNajmabadi mentioned this pull request Mar 31, 2018

Extract out the common portions of the Json and Regex parsing subsystems. #25541

Merged

jcouv added the Area-IDE label Apr 1, 2018

CyrusNajmabadi added 4 commits April 1, 2018 15:14

Merge branch 'regexParsing4' into regexTests

66e14af

Invert check.

1e6fcd9

Merge branch 'regexParsing4' into regexTests

decb512

Merge branch 'regexParsing4' into regexTests

cdb49d7

CyrusNajmabadi changed the base branch from master to features/embeddedRegex April 2, 2018 19:03

CyrusNajmabadi added 3 commits April 2, 2018 14:08

Merge branch 'regexParsing4' into regexTests

e9395ba

Add test.

c11c5bd

Merge branch 'regexParsing4' into regexTests

17b89fd

jcouv reviewed Apr 4, 2018

View reviewed changes

Make tests opt into negative behavior.

02616f6

jcouv reviewed Aug 22, 2018

View reviewed changes

CyrusNajmabadi added 3 commits August 22, 2018 14:45

remove code.

3e34509

Add comment.

ec8960c

Remove extra tests.

6055bcc

Add comments.

63ac94f

jcouv approved these changes Aug 22, 2018

View reviewed changes

Add test info.

e4dab46

CyrusNajmabadi added 4 commits August 22, 2018 15:24

Update test gen.

b6de049

Update basic tests.

192629a

Update neg tests.

cd5c79f

Update tests.

296d6e3

jcouv approved these changes Aug 22, 2018

View reviewed changes

jcouv merged commit 8a731ba into dotnet:features/embeddedRegex Aug 23, 2018

CyrusNajmabadi deleted the regexTests branch January 25, 2020 00:46

Conversation

CyrusNajmabadi commented Mar 16, 2018

Uh oh!

CyrusNajmabadi commented Mar 16, 2018

Uh oh!

jcouv Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 4, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 4, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 4, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 4, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Aug 22, 2018

Uh oh!

jcouv commented Aug 22, 2018

Uh oh!

jcouv commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 22, 2018

Uh oh!

CyrusNajmabadi commented Aug 22, 2018

Uh oh!

CyrusNajmabadi commented Aug 22, 2018

Uh oh!

CyrusNajmabadi commented Aug 22, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 22, 2018

Uh oh!

jcouv Apr 4, 2018 •

edited

Loading

CyrusNajmabadi Apr 4, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 4, 2018 •

edited by jcouv

Loading

jcouv Apr 4, 2018 •

edited

Loading

CyrusNajmabadi Apr 4, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 4, 2018 •

edited by jcouv

Loading

jcouv Aug 16, 2018 •

edited

Loading

CyrusNajmabadi Aug 16, 2018 •

edited by jcouv

Loading

jcouv Aug 22, 2018 •

edited

Loading

CyrusNajmabadi Aug 22, 2018 •

edited by jcouv

Loading

jcouv commented Aug 22, 2018 •

edited

Loading

jcouv commented Aug 22, 2018 •

edited

Loading

jcouv commented Aug 22, 2018 •

edited

Loading