Serve route-snapper file locally for tests#653
Conversation
Sparrow0hawk
force-pushed
the
local-tests
branch
from
2ff3134 to
45a8409
Compare
|
Without the parallelism - these tests seem to be fine for me, but that said it might still be worth pulling this just to be sure in case something slows down - this is a fix that fixed flakiness on main for me |
|
I'd prefer to avoid applying specific timeouts to individual tests, that for me feels like a smell that something isn't working as we'd like if particular parts of the page are taking too long to load. Did you find this branch was flaky? If so let me know so I can test it some more. |
|
Well, they fail with timeouts for me if I reintroduce parallelisation, so it may be 6 of one, half a dozen of the other for us to switch off paralellisation to make the timeouts not happen, vs having paralellisation and a longer timeout |
|
I guess the answer may be - do some meta-tests of what takes longer? The route snapper loads plenty fast on PYATS itself, so I think these tests aren't testing how long the page takes to load, they're testing other features. With that in mind whatever gives us fastest running test suite is the best option, no? |
|
I am keen to avoid parallelism as it potentially hides slower running tests which is something we want to identify and address. In CI tests runs as quickly with this branch as they do on previous PRs on main (around 2mins) so I don't think we actually gain much from parallelism. If we think the test suite is too slow we probably need to get under the hood of which tests are really slowing us down and why. |
Pete-Y-CS
left a comment
Pete-Y-CS
left a comment
There was a problem hiding this comment.
Added a comment - looks like there's a test hello world we might wanna remove but otherwise looks good!
| express.static(path.join(import.meta.dirname, "route-snappers")), | ||
| ); | ||
|
|
||
| app.get("/", async (req, resp, next) => { |
There was a problem hiding this comment.
This looks like it's just here for testing purposes - do we need/want it?
There was a problem hiding this comment.
We need this so that playwright knows the server is up during tests.
2 participants
This PR attempts to allow the test suite to run without fetching route-snapper files from a remote host.
It includes: