avh4 / elm-program-test

Test Elm programs
https://elm-program-test.netlify.com/
MIT License
93 stars 27 forks source link

Considerable performance regression between 3.6.0 and 3.6.2 #159

Closed Arkham closed 2 years ago

Arkham commented 2 years ago

Hi there! 🤗

I've noticed some performance regressions in the latest version, so I put together this tiny app: https://github.com/Arkham/elm-program-test-perf-regression. It's a basic ellie app and I've added a thousand spans to simulate a larger page. I did a couple of runs of warmup for each version.

With 3.6.2:

$ time elm-test tests/
Compiling > Starting tests

elm-test 0.19.1-revision7
-------------------------

Running 1 test. To reproduce these results, run: elm-test --fuzz 100 --seed 73431419374860 tests/

TEST RUN PASSED

Duration: 10123 ms
Passed:   1
Failed:   0

real    0m10.726s
user    0m1.162s
sys     0m0.579s

With 3.6.1:

$ time elm-test tests/
Compiling > Starting tests

elm-test 0.19.1-revision7
-------------------------

Running 1 test. To reproduce these results, run: elm-test --fuzz 100 --seed 163487936329884 tests/

TEST RUN PASSED

Duration: 652 ms
Passed:   1
Failed:   0

real    0m1.363s
user    0m1.652s
sys     0m0.670s

With 3.6.0:

$ time elm-test tests/
Compiling > Starting tests

elm-test 0.19.1-revision7
-------------------------

Running 1 test. To reproduce these results, run: elm-test --fuzz 100 --seed 323981078690301 tests/

TEST RUN PASSED

Duration: 414 ms
Passed:   1
Failed:   0

real    0m1.010s
user    0m1.367s
sys     0m0.580s

Could this be related to the new html parser or the new error parser?

avh4 commented 2 years ago

Out of curiosity, how much time is this adding to your actual test suite?


I put your example into ellie: https://ellie-app.com/gQpDyLGbZGza1

Yeah, looks like it is the HTML parser using up the time:

Screen Shot 2022-03-03 at 9 56 51 AM

And in that example it looks like it's ComplexQuery.findButNot -> firstErrorOf (https://github.com/avh4/elm-program-test/blob/5f5dd8cb842230b52c16f525f61c797d930ccf7b/src/ProgramTest/ComplexQuery.elm#L241) that's triggering that.

Screen Shot 2022-03-03 at 9 59 01 AM

I tried to make the HTML parsing be lazy so it's only done when needed (which should mean only when a test fails), but I probably didn't catch that everywhere.

... 🕵️ ...

I think the performance fix would be to make the Html.Parser.Node arguments here https://github.com/avh4/elm-program-test/blob/5f5dd8cb842230b52c16f525f61c797d930ccf7b/src/ProgramTest/TestHtmlParser.elm#L10-L11 be lazy ((() -> Html.Parser.Node)), since ComplexQuery.firstErrorOf ignores those parameters anyway.

avh4 commented 2 years ago

(Also noting that a more ideal fix would be for elm-explorations/test to give access to the parsed HTML that the Query.Single already has so that elm-program-test wouldn't have to manually re-parse it.)

Arkham commented 2 years ago

Oh, it's pretty substantial! Running elm-test for all NRI tests used to take ~40s, now it doesn't seem it wants to terminate :) I never had the patience to let it run before Ctrl-C'ing the whole thing..

avh4 commented 2 years ago

Should be fixed in 3.6.3 https://package.elm-lang.org/packages/avh4/elm-program-test/3.6.3/ / https://github.com/avh4/elm-program-test/pull/161

If it's now good with tests passing, could you also check by breaking one of your tests and make sure it also works okay when there's a failing test?

Arkham commented 2 years ago

Thank you! I can confirm it works great both with failing tests and without :)