Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: Let runTestCase() pretty-print expected TokenStream too #2056

Merged
merged 4 commits into from
Sep 18, 2019

Conversation

Simran-B
Copy link
Contributor

Only the actual token stream was pretty-printed. This makes the helper function pretty-print the expected token stream as well (and the diff too). Also added some \n to separate it from the mocha output.

@RunDevelopment
Copy link
Member

Could you please explain why you did this?

I don't see the benefit. You can see the (usually) formatted token stream in the test file itself. And the calculated difference between two formatted token streams and two non-formatted token streams should be the same (apart from some whitespaces).

Also, a potential problem I see is that the mocha output will be longer which is a bit of a problem because consoles usually only store a fixed number of lines. The output of some more complex test cases can easily exceed 1000 lines because of the (current) diff. (For reference: The number of stored lines in VS Code's terminal is 1000 by default.)
Won't this be even longer with is PR?

@Simran-B
Copy link
Contributor Author

Well, it already formatted the actual token stream. Having it display the actual data pretty-printed but not the expected data seems inconsistent. If you want the terse representation then you can just leave out the --pretty flag. For longer tests I would for sure redirect the output to a file for further inspection.

Based on your argument that you can see the formatted token stream in the test file itself, it shouldn't display the expected data at all, but just the actual data.

Not sure about the usefulness of the diff, it's really just the actual data from the first difference on - if there's an extra character in the very first line of a test then you get the full actual data twice. And mocha actually does a better job at showing a mismatch:

image

@RunDevelopment
Copy link
Member

Having it display the actual data pretty-printed but not the expected data seems inconsistent.

True, but do need to have the expected token stream formatted?
I only added formatting for the actual token stream because I want to be able to copypaste it.

Based on your argument that you can see the formatted token stream in the test file itself, it shouldn't display the expected data at all [...]

Well, yes kinda. I don't think we gain anything from displaying the expected token stream. After all, you can see it in the test file, so you know exactly what's expected.
The only reason I see for its existence is consistency. Test errors usually have this expected vs actual style, so I guess it makes sense to display the expected token stream. I'm not too sure about this myself.

Not sure about the usefulness of the diff, it's really just the actual data from the first difference on - if there's an extra character in the very first line of a test then you get the full actual data twice. And mocha actually does a better job at showing a mismatch:

(I never gave any thought to our diff...) You're exactly right. Mocha does a way better job. We should just remove our diff.


I spent way too muchsome time digging through the git blames and found out that the message was added by this commit from #1041.
But strangely, in the PR it was added by a merge commit.

If I had to guess, @Golmote didn't notice this change before merging (@Golmote wouldn't have accepted code indented this badly) and it just stuck around.

Knowing this: What more than the actual token stream and a small diff from mocha do we really need?
Might as well take this chance to make the errors a little simpler.

@Simran-B
Copy link
Contributor Author

To be honest, I would scratch the diff and the expected output and just print the actual token stream. This could even be behind another flag, as mocha tells you pretty much everything you need to know. But I guess it doesn't hurt as long as it is printed above what mocha outputs.

Not sure if there is anything else needed. The position of the first difference / error? But that's tricky, because the raw as well as the reformatted token streams differ from the test file, so there's no straightforward way to compute that for the file. What could be helpful though is the location of the test in question, which mocha doesn't tell you.

I committed these changes, let me know what you think:

image

(without --pretty)

@RunDevelopment
Copy link
Member

I really like the simplified format.

I added a bit code to also have the line and column number of the first difference at the end of the file path. Most IDEs understand this format, so you can jump directly into the test case at the position of the difference.

image

Please tell me what you think!
@Simran-B @mAAdhaTTah

@Simran-B
Copy link
Contributor Author

I tried it with different white-space in the test file and it seems to point reliably at or behind the position of the deviation. Looks very good to me!

@mAAdhaTTah
Copy link
Member

I dig it. :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants