chore: Refactor unit tests #2294

bartlomieju · 2019-05-05T11:20:14Z

This PR tries to resolve issue #2103.

In the first pass I've changed the behavior of Python script to run unit tests with every possible combination of permissions. This ensures that no test is skipped but requires to spawn separate Deno process for each combination even if there are no tests to run. This approach is really suboptimal.

My next step will be to first run a process to discover all tests and establish which perm combinations are used in test cases and then spawn new processes only for those combinations.

By the way, I think that this combination technique might be useful for users as well. Maybe we should consider implementing it in standard library? Eg. in deno-postgres I need to run tests once with --allow-env flag and second time without it, I'd be neat if this was built in without need to port this behavior.

CC @ry

js/test_util.ts

bartlomieju · 2019-05-05T17:30:01Z

Alright, I got functionality from 1st comment working. It still needs cleanup but most of logic is already there.

EDIT: With removal of testing.setFilter it seems that tests should be run with --no-prompt now

ry · 2019-05-05T19:13:19Z

tools/unit_tests.py

-        raise AssertionError("Bad js/unit_test.ts output")
-    if expected != actual:
-        print "expected", expected, "actual", actual
-        raise AssertionError("expected tests did not equal actual")


This logic should be ported over to the runner too

The thing is: up till now if one of tests failed remaining tests for other permissions were not run. I think we can safely spawn subprocesses for each permissions combination and return failure if any of them failed. That way all tests will always run, what do you think?

That sounds fine. But for each set of permissions there is a summary line printed. For example:

Running tests for: --allow-write --allow-run running 1 tests test runWithCwdIsAsync ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

The js/unit_tests_runner.ts needs to parse the first line (to determine how many tests there are) and then also parse the last line to check that passed matches. If we don't have this feature, it's possible for Deno to silently crash with exit code 0 and the tests all pass.

You're right, I'll add it

Okay, so I've ported it to TS, but the thing is... There is no line-per-line output of unit tests ATM because there's no available lines helper. I didn't want to copy over chunks implemented by @kevinkassimo from xeval.ts and there's no way to use it, because that file uses extensionless imports.

Just copy it over add a todo ?

Okay I tried... It's not really possible since chunk from xeval.ts uses import from js/buffer.ts so again there's conflict because of extensionless imports. I'm gonna leave it as is for now...

js/unit_test_runner.ts

js/test_util.ts

js/unit_test_runner.ts

bartlomieju · 2019-05-06T22:17:27Z

I've also added short summary at the end of run, because it's easy to miss if something went wrong at the beginning:

$ ./tools/unit_tests.py target/debug/deno

...

Summary for <no permissions>
test result: ok. 216 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-read
test result: FAILED. 33 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-net
test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-read --allow-write
test result: ok. 48 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-write
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-env
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-run
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-write --allow-run
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Summary for --allow-high-precision
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Unit tests failed

What do you think?

ry · 2019-05-06T22:21:41Z

I'm slightly concerned around running many processes in parallel on windows. @piscisaureus can you please check this branch on windows works for you:

# Run http server in the brackground first
./tools/http_server.py &  

./target/debug/deno run -A js/unit_test_runner.ts

ry · 2019-05-06T22:24:10Z

Otherwise looks great!

tools/unit_tests.py

bartlomieju · 2019-05-06T22:36:10Z

I'm slightly concerned around running many processes in parallel on windows.

Processes are run serially. Also command you showed is unnecessary, you can just run tools/unit_tests.py

There's also one more thing to solve, please see this comment

Otherwise looks great!

Thanks!

piscisaureus · 2019-05-06T22:54:30Z

I get this (freshly rebuilt):

D:\deno2>python tools\unit_tests.py target\release\deno
Deno test server http://localhost:4545/
redirect server http://localhost:4546/ -> http://localhost:4545/
redirect server http://localhost:4547/ -> http://localhost:4545/
redirect server http://localhost:4548/ -> http://localhost:4546/
Compiling file:///D:/deno2/js/unit_test_runner.ts
Compiling file:///D:/deno2/js/test_util.ts
D:/deno2/js/test_util.ts:225:66 - error TS1064: The return type of an async function or method must be the global Promise<T> type.

225 testPerm({ read: true }, async function parsingUnitTestOutput(): void {
                                                                     ~~~~

bartlomieju · 2019-05-06T22:58:28Z

@piscisaureus I've just pushed a fix

piscisaureus · 2019-05-06T23:00:51Z

D:\deno2>python tools\unit_tests.py target\release\deno
Deno test server http://localhost:4545/
redirect server http://localhost:4546/ -> http://localhost:4545/
redirect server http://localhost:4547/ -> http://localhost:4545/
redirect server http://localhost:4548/ -> http://localhost:4546/
Compiling file:///D:/deno2/js/unit_test_runner.ts
<...snip...>
Compiling file:///D:/deno2/js/deps/https/deno.land/std/testing/format.ts
Discovered permission combinations for tests: 9
        <no permissions>
        --allow-read
        --allow-net
        --allow-read --allow-write
        --allow-write
        --allow-env
        --allow-run
        --allow-write --allow-run
        --allow-high-precision
Running tests for: <no permissions>
Assertion failedHello worldHello world
<...process hangs...>

bartlomieju · 2019-05-07T08:44:40Z

@piscisaureus would you be willing to help out with debugging? Unfortunately I don't have access to Windows machine

zekth · 2019-05-08T12:47:28Z

js/unit_test_runner.ts

+      stdout: "piped"
+    });
+
+    await p.status();


This makes the windows env hangs.
Commenting it make everything works as we wait for p.output() after. Maybe this is a bug in the Deno.run ?

@zekth kudos for help 🙏 I'll update PR in the evening

@zekth Can you create an issue/test case for the Deno.run error? (assuming it's a bug!)

@hayd spent 1 hour trying to reproduce this, no luck for the moment. Raising an issue to track it.

zekth · 2019-05-08T13:03:56Z

Additionnal note:
with disabling await p.status() the tests passe. BUT there is a strange behavior, if you check the ouput of my tests: https://gist.github.com/zekth/8af9973a94c1fd541869e6ab901f00d1
there is a difference between the mac os one for example : https://travis-ci.com/denoland/deno/jobs/198071750

TLDR: there is no console test shown in my logs. But we have the same output : test result: ok. 216 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

As i mentionned i've supposed a problem in the Deno.run(). Tried the example of the manual : https://deno.land/manual.html#runsubprocess with the check of process.status(). And it works. Seems like the exception in the blob test may throw something not interpreted the same way on all envs.

bartlomieju · 2019-05-08T22:41:09Z

Okay so removing await p.status() resolved problem on Windows. Not really sure what was the problem, it will need some investigating.

zekth · 2019-05-08T23:00:06Z

Okay so removing await p.status() resolved problem on Windows. Not really sure what was the problem, it will need some investigating.

As this PR may land before resolving this behaviour it is revelant for me to raise an issue for this to track before having some other problems like this on windows

ry · 2019-05-08T23:11:28Z

js/unit_test_runner.ts

+
+    let result = 0;
+
+    if (!actual && !expected) {


This is a little difficult to read. I presume this is covering the case where actual and expected are both undefined? It could use a comment.

ry

This looks great! Thanks for seeing this through.

first pass at refactoring unit tests

5e73a6f

zekth reviewed May 5, 2019

View reviewed changes

js/test_util.ts Outdated Show resolved Hide resolved

bartlomieju added 3 commits May 5, 2019 13:33

remove old perm stringifaction

a629a8e

spanw HTTP server when running unit tests

b229771

spawn deno subprocesses for unit tests

d57b46f

lint

f5c07f3

bartlomieju force-pushed the chore_refactor_unit_tests branch from 0daa669 to f5c07f3 Compare May 5, 2019 17:49

fix appveyor

8d1dd21

ry reviewed May 5, 2019

View reviewed changes

bartlomieju added 3 commits May 5, 2019 22:46

cleanup

92a9ab4

add necessary --no-prompt

d5ef2fa

lint

f252b0e

bartlomieju commented May 6, 2019

View reviewed changes

js/test_util.ts Show resolved Hide resolved

bartlomieju added 4 commits May 6, 2019 22:29

verify output of unit tests

3a9b42b

cleanup js/test_util.ts

89ac08c

cleanup tools/unit_tests.py

c331dd0

lint

db7a45f

ry reviewed May 6, 2019

View reviewed changes

js/unit_test_runner.ts Outdated Show resolved Hide resolved

bartlomieju added 3 commits May 7, 2019 00:00

add test for parseUnitTestOutput

26a1507

remove parse_unit_test_output

51797b0

add summary for unit tests

5f66e6a

ry reviewed May 6, 2019

View reviewed changes

tools/unit_tests.py Outdated Show resolved Hide resolved

refactor tools/unit_tests.py

dc34831

fix return type

d9ae36c

zekth suggested changes May 8, 2019

View reviewed changes

bartlomieju added 2 commits May 9, 2019 00:08

don't await unit test status

98f936c

fmt

1ec2783

zekth mentioned this pull request May 8, 2019

[bug] windows : deno.run.status #2315

Closed

ry reviewed May 8, 2019

View reviewed changes

ry approved these changes May 8, 2019

View reviewed changes

ry merged commit ac8c6fe into denoland:master May 8, 2019

This was referenced May 9, 2019

WIP Try hanging process test on windows #2323

Closed

Port tools/unit_tests.py to deno #1213

Closed

bartlomieju deleted the chore_refactor_unit_tests branch May 19, 2019 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Refactor unit tests #2294

chore: Refactor unit tests #2294

bartlomieju commented May 5, 2019

bartlomieju commented May 5, 2019 •

edited

ry May 5, 2019

bartlomieju May 5, 2019 •

edited

ry May 6, 2019

bartlomieju May 6, 2019

bartlomieju May 6, 2019

bartlomieju May 6, 2019

ry May 6, 2019

bartlomieju May 7, 2019

bartlomieju May 8, 2019

bartlomieju commented May 6, 2019

ry commented May 6, 2019 •

edited

ry commented May 6, 2019

bartlomieju commented May 6, 2019 •

edited

piscisaureus commented May 6, 2019

bartlomieju commented May 6, 2019

piscisaureus commented May 6, 2019

bartlomieju commented May 7, 2019

zekth May 8, 2019

bartlomieju May 8, 2019

hayd May 8, 2019

zekth May 8, 2019

zekth commented May 8, 2019

bartlomieju commented May 8, 2019 •

edited

zekth commented May 8, 2019

ry May 8, 2019

ry left a comment

chore: Refactor unit tests #2294

chore: Refactor unit tests #2294

Conversation

bartlomieju commented May 5, 2019

bartlomieju commented May 5, 2019 • edited

Choose a reason for hiding this comment

bartlomieju May 5, 2019 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented May 6, 2019

ry commented May 6, 2019 • edited

ry commented May 6, 2019

bartlomieju commented May 6, 2019 • edited

piscisaureus commented May 6, 2019

bartlomieju commented May 6, 2019

piscisaureus commented May 6, 2019

bartlomieju commented May 7, 2019

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

zekth commented May 8, 2019

bartlomieju commented May 8, 2019 • edited

zekth commented May 8, 2019

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

bartlomieju commented May 5, 2019 •

edited

bartlomieju May 5, 2019 •

edited

ry commented May 6, 2019 •

edited

bartlomieju commented May 6, 2019 •

edited

bartlomieju commented May 8, 2019 •

edited