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

Merge tests into a larger file (part 1 of ∞) #9318

Merged
merged 17 commits into from
Oct 12, 2018

Conversation

narimiran
Copy link
Member

As proposed here and approved here.

This is a veeeery long (and boring) task, so I decided to split it into multiple parts as I go.

I started going alphabetically, so if anybody wants to join — you can go from the other side towards me.

@bluenote10
Copy link
Contributor

Is that really a good idea? Now the expected output blocks are huge and when a single check fails the test result will print two huge blocks of expected and actual output and it is time consuming to diff the blocks just to see which check fails.

Maybe the general problem is that verifying tests via an expected stdout output does not scale well, because the check conditions cannot easily be linked to the corresponding code -- neither for a human reader nor for testament.

@narimiran
Copy link
Member Author

Now the expected output blocks are huge and when a single check fails the test result will print two huge blocks of expected and actual output and it is time consuming to diff the blocks just to see which check fails.

Could this be remedied by using asserts instead of echoing?

@dom96
Copy link
Contributor

dom96 commented Oct 11, 2018

It can be remedied by a simple diff performed by testament and shown to the user. It's not ideal though and I'm also not sure we should be merging tests like this.

It may be better to set up a proper CI infrastructure for Nim. But that requires a lot of commitment from us.

@narimiran
Copy link
Member Author

output blocks are huge

tclosure.nim seems like a biggest offender here, with 70 output lines. I'll see what I can do about it. (either split it into a several files or some other solution)

I'm also not sure we should be merging tests like this.

There are lots of very short test which output zero or one line. I'll concentrate on merging these, and leave the tests which already have multiline output as they are.

@kaushalmodi
Copy link
Contributor

what about unittest? That allows all tests to be in a single file and shows which exact test failed.

@Araq
Copy link
Member

Araq commented Oct 11, 2018

what about unittest? That allows all tests to be in a single file and shows which exact test failed.

No unittest in my tests.

@Araq
Copy link
Member

Araq commented Oct 11, 2018

Could this be remedied by using asserts instead of echoing?

Yes, that is what should be done indeed.

Use `doAssert` where possible.
@narimiran
Copy link
Member Author

In the recent commit I've used asserts where it was possible — output blocks are now much smaller, and should be easier to spot differences between the expected and actual output.

I'll start working on the second batch of these if/when this is merged.

@Araq
Copy link
Member

Araq commented Oct 12, 2018

This is mostly fine, but you're overdoing it a bit, the "concepts" tests should remain splitted, concepts are actively being worked on, same for tcomputedgoto. In general don't merge tests that are over 100 lines.

But all in all, excellent job, I hope it will help us with CI timeouts!

krux02 pushed a commit to krux02/Nim that referenced this pull request Oct 15, 2018
* merge actiontable tests

* merge arithm tests

* merge array tests

* merge assign tests

* merge bind tests

* merge casestmt tests

* merge closure tests

* merge cnt seq tests

* merge collections tests

* merge concept issues tests

* merge concept tests

* fix failing tests

* smaller outputs

Use `doAssert` where possible.

* fix wrong output

* split `tcomputedgoto`

* revert merging concepts

* fix failing test
narimiran added a commit to narimiran/Nim that referenced this pull request Oct 31, 2018
* merge actiontable tests

* merge arithm tests

* merge array tests

* merge assign tests

* merge bind tests

* merge casestmt tests

* merge closure tests

* merge cnt seq tests

* merge collections tests

* merge concept issues tests

* merge concept tests

* fix failing tests

* smaller outputs

Use `doAssert` where possible.

* fix wrong output

* split `tcomputedgoto`

* revert merging concepts

* fix failing test
narimiran added a commit that referenced this pull request Nov 1, 2018
* merge actiontable tests

* merge arithm tests

* merge array tests

* merge assign tests

* merge bind tests

* merge casestmt tests

* merge closure tests

* merge cnt seq tests

* merge collections tests

* merge concept issues tests

* merge concept tests

* fix failing tests

* smaller outputs

Use `doAssert` where possible.

* fix wrong output

* split `tcomputedgoto`

* revert merging concepts

* fix failing test

(cherry picked from commit 7f18d7c)
@narimiran narimiran deleted the test-merger branch November 12, 2018 15:16
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.

5 participants