Fix for issue 1836 #2027

MagnumOpus21 · 2018-10-19T00:27:43Z

Fixes #1836.
@ramya-rao-a
These are the images I get after making the modifications.
This image is that of a passing test case.

This image is for a failing test case.

This is for one passing case and one failing case. (Package wide)
.
I have tested this with all the options from the command palette.
Do tell me if I need to add more to the PR, to make it even better :)

ramya-rao-a

@MagnumOpus21 Thanks for the PR. I believe there has been some misunderstanding on the issue that we are trying to fix here.

Below is a picture of your second case.

The text rounded up in red is the bug. When you run the test in the terminal, you get the timestamp followed by the file details. This line should not be picked up by expandFilePathInOutput

Only lines in the output that correspond to actual test failure should be picked up by expandFilePathInOutput

MagnumOpus21 · 2018-10-20T23:07:58Z

Ah my bad. I thought the "prepended" by --- FAIL : was a mistake. I shall rectify this in the following commit.

MagnumOpus21 · 2018-10-21T02:19:56Z

I modified the regex inside expandPathLinesOutput.

Why

When golang prints something to the console, it appends the timestamp with the file path to the console inside the tests. For the failures, it doesn't have this stated behavior. So, the regex now only captures failure of test cases since they don't have the timestamp at the beginning of the output.
Also, file names are supposed to be in snake_case and the regex captures this.

How I arrived at this conclusion

I noticied that I was getting the lines, one by one, instead of having them all at once. I couldn't access the previous lines. I printed it to the console, and found this out. I have no idea why previously I could check for the next line (lines[i + 1]).
When I tried this for lines[i - 1] it didn't process the lines at all. (Because we have one line and the value at i -1 th index is undefined). If I am missing something do correct me. This is my very first experience working with TypeScript.

Output with this commit.

I believe this was the intended fix. I apologize for not getting the correct behavior initially.

ramya-rao-a · 2018-10-21T21:53:51Z

src/testUtils.ts

@@ -340,7 +340,7 @@ export function cancelRunningTests(): Thenable<boolean> {
 function expandFilePathInOutput(output: string, cwd: string): string {
 	let lines = output.split('\n');
 	for (let i = 0; i < lines.length; i++) {
-		let matches = lines[i].match(/^\s*(.+.go):(\d+):/);
+		let matches = lines[i].match(/^\s*(\s*\w+.go):(\d+):/);


Why do we want to capture the spaces here? Is this to cater to the case when the file name contains a space?

I thought spaces might be present at the beginning of the file. This has another bug, where if we have spaces in the filename like file name.go the regex will fail to pick this. Another commit inc.

Add a compile error in your code and then run the test. The test output will now show the build error in the below format:
.\reverse_test.go:23:12: undefined: sring

Now, with the current change, the above will not get expanded because \w will not match .\

MagnumOpus21 · 2018-10-21T23:05:22Z

Also, I notice that the hyperlinks break after a space inside the output terminal in vscode. Is this the expected behavior?

ramya-rao-a · 2018-10-22T16:43:38Z

@MagnumOpus21 Are you sure the last commit makes a difference?

File names with spaces would have worked even without that commit as far as I understand.

Since that is a very corner case, I am ok not supporting the scenario where file name has spaces.
I am more concerned about #2027 (comment)

MagnumOpus21 · 2018-10-22T17:44:22Z

The GitHub outage prevented me from viewing your previous message with the regex for the build errors. I could see your why not this regex suggestion earlier in the day, now I am not able to spot it at all. Still am not sure if I can see all the suggestions after the outage started on outdated thread 😅.

MagnumOpus21 · 2018-10-22T18:15:20Z

@ramya-rao-a Now it should capture all the failures on non-Windows platforms as well. I was just testing the extension on Windows. 😓

ramya-rao-a · 2018-10-23T03:09:00Z

src/testUtils.ts

@@ -340,7 +340,7 @@ export function cancelRunningTests(): Thenable<boolean> {
 function expandFilePathInOutput(output: string, cwd: string): string {
 	let lines = output.split('\n');
 	for (let i = 0; i < lines.length; i++) {
-		let matches = lines[i].match(/^\s*(.+.go):(\d+):/);
+		let matches = lines[i].match(/^\s*([(\.\/)?\w+\s*]+.go):(\d+):/);


Things inside the [] dont need the quantifiers like ?, + or *. [] defines the allowed characters not their quantity or order.

Also, this will not work for Windows.

Can you make this an exported function and add some tests to cover the different scenarios?

You can add the tests in https://github.com/Microsoft/vscode-go/blob/master/test/unit/utils.test.ts

MagnumOpus21 · 2018-11-12T03:42:30Z

Sorry for the no progress during the past weeks. Hectic school session in progress. I will finish the PR during Thanksgiving week. Thanks for your patience.

ramya-rao-a · 2018-11-13T02:12:43Z

No problem @MagnumOpus21, take your time.

peergynt · 2019-02-14T21:14:51Z

@MagnumOpus21 did you see my comment in the original issue (#1836) by any chance?
It seems like checking that the line starts with at least a tab or four spaces could help.

MagnumOpus21 · 2019-02-14T21:31:23Z

I'm sorry 😔. I was busy with school. I'll take a look at this and finish this fast. It's been far too long. Apologize for the lateness @peergynt

msftclas · 2019-03-12T06:20:24Z

All CLA requirements met.

MagnumOpus21 · 2019-03-13T00:36:45Z

@ramya-rao-a Do I have to do anything to get the Go Extension tests to pass for go1.11. It passes for the other versions of golang on Travis.

ramya-rao-a · 2019-10-06T21:57:09Z

Hey @MagnumOpus21

My apologies for not getting to this sooner

I tried a few more cases with the code in this PR

some random things C:/my/folder/fine.go:21: fails as expected because the path is already expanded
some random things /my/folder/fine.go:21: does not fail even though the path is an absolute path in non Windows machines.

I am starting to feel that we are getting into risky areas by trying to cater to extract the right relative path that needs to be expanded.

We know what the expanded path needs to look like because we have access to cwd, so I am leaning towards checking the existence of cwd in the output line should be taken as a sign as to not to bother expanding the file path.

Thoughts?

ramya-rao-a · 2020-03-26T06:17:24Z

Hey @MagnumOpus21

Since working with regexes still left some corner cases not covered, I've decided to take a different approach here. Please see #1836 (comment)

Thanks for all the work here & Happy Coding!

MagnumOpus21 added 2 commits October 18, 2018 20:16

Fix for issue 1836

ff1c461

Updated testUtils according to tslinter suggestions.

865bfbf

ramya-rao-a suggested changes Oct 20, 2018

View reviewed changes

Modifed regex to look for file name without timestamp.

693b78d

ramya-rao-a reviewed Oct 21, 2018

View reviewed changes

Filenames with spaces are now considered in the regex match.

9f00d7b

Regex modified to encompass build failures on *nix platforms

8e40205

ramya-rao-a reviewed Oct 23, 2018

View reviewed changes

MagnumOpus21 added 3 commits March 12, 2019 02:03

Merge branch 'master' of https://github.com/Microsoft/vscode-go

ec62eb3

Modifed regex to work on Windows platforms, function is exported.

2ce2547

Added tests for expandFilePathInOutput

67c392e

ramya-rao-a closed this Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 1836 #2027

Fix for issue 1836 #2027

MagnumOpus21 commented Oct 19, 2018

ramya-rao-a left a comment

MagnumOpus21 commented Oct 20, 2018

MagnumOpus21 commented Oct 21, 2018 •

edited

Loading

ramya-rao-a Oct 21, 2018

MagnumOpus21 Oct 21, 2018

ramya-rao-a Oct 21, 2018

MagnumOpus21 commented Oct 21, 2018

ramya-rao-a commented Oct 22, 2018

MagnumOpus21 commented Oct 22, 2018 •

edited

Loading

MagnumOpus21 commented Oct 22, 2018

ramya-rao-a Oct 23, 2018

MagnumOpus21 commented Nov 12, 2018

ramya-rao-a commented Nov 13, 2018

peergynt commented Feb 14, 2019

MagnumOpus21 commented Feb 14, 2019

msftclas commented Mar 12, 2019 •

edited

Loading

MagnumOpus21 commented Mar 13, 2019

ramya-rao-a commented Oct 6, 2019

ramya-rao-a commented Mar 26, 2020

Fix for issue 1836 #2027

Fix for issue 1836 #2027

Conversation

MagnumOpus21 commented Oct 19, 2018

ramya-rao-a left a comment

Choose a reason for hiding this comment

MagnumOpus21 commented Oct 20, 2018

MagnumOpus21 commented Oct 21, 2018 • edited Loading

Why

How I arrived at this conclusion

Output with this commit.

ramya-rao-a Oct 21, 2018

Choose a reason for hiding this comment

MagnumOpus21 Oct 21, 2018

Choose a reason for hiding this comment

ramya-rao-a Oct 21, 2018

Choose a reason for hiding this comment

MagnumOpus21 commented Oct 21, 2018

ramya-rao-a commented Oct 22, 2018

MagnumOpus21 commented Oct 22, 2018 • edited Loading

MagnumOpus21 commented Oct 22, 2018

ramya-rao-a Oct 23, 2018

Choose a reason for hiding this comment

MagnumOpus21 commented Nov 12, 2018

ramya-rao-a commented Nov 13, 2018

peergynt commented Feb 14, 2019

MagnumOpus21 commented Feb 14, 2019

msftclas commented Mar 12, 2019 • edited Loading

MagnumOpus21 commented Mar 13, 2019

ramya-rao-a commented Oct 6, 2019

ramya-rao-a commented Mar 26, 2020

MagnumOpus21 commented Oct 21, 2018 •

edited

Loading

MagnumOpus21 commented Oct 22, 2018 •

edited

Loading

msftclas commented Mar 12, 2019 •

edited

Loading