Normalize line separators for .txt files (fix Windows tests) #3488

KvanTTT · 2022-01-11T17:34:08Z

No description provided.

KvanTTT · 2022-01-11T21:11:26Z

I still don't understand how to force \n line separators for Windows instead of \r\n. Git doesn't accept the change of separators. All tests expect \n line separator.

ericvergnaud · 2022-01-11T22:03:16Z

Have you tried: git config --global core.autocrlf false
See https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

KvanTTT · 2022-01-12T11:38:57Z

Yes, I've tried with no effect. Ok, I'll restore line separators processing and replace line separators tests with handwritten tests.

parrt · 2022-01-13T01:49:30Z

Oh sorry. didn't see this yet, @KvanTTT (Just emailed you).

KvanTTT · 2022-01-13T10:08:55Z

Yes, I'm working on it.

KvanTTT · 2022-01-15T12:29:33Z

@parrt the fix is ready.

KvanTTT · 2022-01-15T12:32:10Z

runtime-testsuite/test/org/antlr/v4/test/runtime/ExtraTests.java

+import java.util.List;
+
+public class ExtraTests {
+	public static RuntimeTestDescriptor[] getRuntimeTestDescriptors(String targetName) {


These tests are useful. We can write here test for generated and very large grammars. For instance, LargeLexer.txt can be moved here.

Why not include LargeLexer etc... in main section?

Can you remind me what the purpose is for a new file ExtraTests.java ?

Is it only way to distinguish \r\n from \n tests?

Why not include LargeLexer etc... in main section?

Because it contains a lot of duplicated lines. It can be replaced by a short and clear test with several lines (also, it's possible to vary the number of tokens and other parameters).

Is it only way to distinguish \r\n from \n tests?

For now yes. But I'm working on #1863 and other related bugs that also require big tests that ideally should be generated.

I'll try to think about this today. One first thought. We don't need the regex anymore if we use \n. split works great and is much easier to understand. Also means less change to existing code in a PR.

Ok, I'll remove regex. Definitely, now it does not make sense since we have other tests for line endings checking.

Done. Please don't squash my commits since they are logically different.

Also, in the future, we can use use fixup! commits for easier changes review. Before the final PR merge, all !fixup are being merged to original commits for clear history. I mean !fixup Change something is being merged to Change something.

hi. the fixup thing might be hard for me to remember and complicate my life ;)

Ok. I just always use interactive rebase in IDEA and never have troubles :)

KvanTTT · 2022-01-16T12:51:42Z

runtime-testsuite/test/org/antlr/v4/test/runtime/ExtraTests.java

+		return result;
+	}
+
+	static RuntimeTestDescriptor getLargeLexerDescriptor(String targetName) {


Ok, I've moved LargeLexer in the current PR for the demo.

Ok. Yeah, I'm still thinking we are making a big departure from the point of descriptor files to solve this \r\n thing. Seems we're off the road to shove in a test case for a tiny fraction of the number of tests

I have yet another generated test getAtnStatesSizeMoreThan65535Descriptor in another PR. It's too excess to use yet another big file with uniform data. Output data is also being generated.

I see. yes, starting to make sense to me for generated examples.

parrt · 2022-01-17T20:10:55Z

Ok, looking good now in my mind except that we've so far tried to group tests now by area like lexing, parsing. Instead of adding new TestExtra, shouldn't we add the new tests to TestLexer or whatever?

KvanTTT · 2022-01-17T20:18:12Z

Ok, maybe it makes sense. I just didn't want to mix descriptor and handwritten tests but probably it does not matter.

parrt · 2022-01-17T20:21:31Z

Ok, maybe it makes sense. I just didn't want to mix descriptor and handwritten tests but probably it does not matter.

yeah, probably better to mix gen vs files to keep test topics/areas the same. This way we can also avoid adding new TestX.java file

parrt · 2022-01-17T20:21:54Z

Seems github is hosed. This times out in actions: Download action repository 'stCarolas/setup-maven@v4' (SHA:1d56b37995622db66cce1214d81014b09807fb5a) Ah. it's moving now.

…s (LineSeparatorLf, LineSeparatorCrLf)

KvanTTT · 2022-01-17T20:34:26Z

yeah, probably better to mix gen vs files to keep test topics/areas the same. This way we can also avoid adding new TestX.java file

Indeed, much fewer changes.

Seems github is hosed. This times out in actions: Download action repository 'stCarolas/setup-maven@v4' (SHA:1d56b37995622db66cce1214d81014b09807fb5a)

That's probably a problem with self-hosted CI again. I see the following:

Error: The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing.

parrt · 2022-01-17T21:42:27Z

@ericvergnaud does the antlr data center look ok from your side? Any network issues? thanks!

parrt · 2022-01-17T21:45:32Z

runtime-testsuite/test/org/antlr/v4/test/runtime/BaseRuntimeTest.java

@@ -367,6 +366,13 @@ public static ErrorQueue antlrOnString(String workdir,
 				System.err.println("Can't read descriptor file "+fname);
 			}
 		}
+


this makes me kinda nervous but better than altering all target test files I guess.

parrt · 2022-01-17T21:46:32Z

runtime-testsuite/test/org/antlr/v4/test/runtime/ExtraTests.java

@@ -0,0 +1,68 @@
+package org.antlr.v4.test.runtime;
+
+public class ExtraTests {


Sorry to be fussy but "Extra" isn't very specific. Maybe "GeneratedTests" or even "GeneratedLexerDescriptors"?

I'll merge then think/tweak of name

Yes, but I haven't found a better name. Generated is also not very correct since it contains line separator tests. Maybe handwritten or something like this.

parrt · 2022-01-17T21:49:36Z

Thanks for the hard work, @KvanTTT !

ericvergnaud · 2022-01-17T22:02:59Z

@ericvergnaud does the antlr data center look ok from your side? Any network issues? thanks!

No apparent issue here, some builds are successful.

KvanTTT force-pushed the line-separator-fixes branch from 1e94a77 to bfb7efc Compare January 14, 2022 20:27

KvanTTT marked this pull request as ready for review January 14, 2022 21:34

KvanTTT commented Jan 15, 2022

View reviewed changes

KvanTTT mentioned this pull request Jan 15, 2022

Get rid of ATN serialization restrictions (ATN states size can be > 65535, up to 2^31-1), remove excess serialization and allocations #3493

Closed

KvanTTT commented Jan 16, 2022

View reviewed changes

KvanTTT force-pushed the line-separator-fixes branch from 9021548 to 905001b Compare January 17, 2022 19:29

parrt added the comp:testing label Jan 17, 2022

parrt added this to the 4.10 milestone Jan 17, 2022

KvanTTT added 2 commits January 17, 2022 23:28

Restore \n for all input in runtime tests, add extra LexerExec test…

eed50cf

…s (LineSeparatorLf, LineSeparatorCrLf)

Add generated LargeLexer test, remove LargeLexer.txt descriptor

40ebc46

KvanTTT force-pushed the line-separator-fixes branch from 905001b to 40ebc46 Compare January 17, 2022 20:32

parrt reviewed Jan 17, 2022

View reviewed changes

parrt merged commit 6034525 into antlr:master Jan 17, 2022

KvanTTT deleted the line-separator-fixes branch January 17, 2022 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize line separators for .txt files (fix Windows tests) #3488

Normalize line separators for .txt files (fix Windows tests) #3488

KvanTTT commented Jan 11, 2022

KvanTTT commented Jan 11, 2022

ericvergnaud commented Jan 11, 2022

KvanTTT commented Jan 12, 2022 •

edited

Loading

parrt commented Jan 13, 2022

KvanTTT commented Jan 13, 2022

KvanTTT commented Jan 15, 2022

KvanTTT Jan 15, 2022

parrt Jan 15, 2022

parrt Jan 15, 2022

parrt Jan 15, 2022

KvanTTT Jan 15, 2022

parrt Jan 17, 2022 •

edited

Loading

KvanTTT Jan 17, 2022

KvanTTT Jan 17, 2022 •

edited

Loading

parrt Jan 17, 2022

KvanTTT Jan 17, 2022

KvanTTT Jan 16, 2022

parrt Jan 16, 2022

KvanTTT Jan 16, 2022 •

edited

Loading

parrt Jan 17, 2022

parrt commented Jan 17, 2022 •

edited

Loading

KvanTTT commented Jan 17, 2022

parrt commented Jan 17, 2022

parrt commented Jan 17, 2022 •

edited

Loading

KvanTTT commented Jan 17, 2022 •

edited

Loading

parrt commented Jan 17, 2022

parrt Jan 17, 2022

parrt Jan 17, 2022

parrt Jan 17, 2022

KvanTTT Jan 17, 2022

parrt commented Jan 17, 2022

ericvergnaud commented Jan 17, 2022

		@@ -0,0 +1,68 @@
		package org.antlr.v4.test.runtime;

		public class ExtraTests {

Normalize line separators for .txt files (fix Windows tests) #3488

Normalize line separators for .txt files (fix Windows tests) #3488

Conversation

KvanTTT commented Jan 11, 2022

KvanTTT commented Jan 11, 2022

ericvergnaud commented Jan 11, 2022

KvanTTT commented Jan 12, 2022 • edited Loading

parrt commented Jan 13, 2022

KvanTTT commented Jan 13, 2022

KvanTTT commented Jan 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parrt Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KvanTTT Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KvanTTT Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parrt commented Jan 17, 2022 • edited Loading

KvanTTT commented Jan 17, 2022

parrt commented Jan 17, 2022

parrt commented Jan 17, 2022 • edited Loading

KvanTTT commented Jan 17, 2022 • edited Loading

parrt commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parrt commented Jan 17, 2022

ericvergnaud commented Jan 17, 2022

KvanTTT commented Jan 12, 2022 •

edited

Loading

parrt Jan 17, 2022 •

edited

Loading

KvanTTT Jan 17, 2022 •

edited

Loading

KvanTTT Jan 16, 2022 •

edited

Loading

parrt commented Jan 17, 2022 •

edited

Loading

parrt commented Jan 17, 2022 •

edited

Loading

KvanTTT commented Jan 17, 2022 •

edited

Loading