test: run tests on multiple node versions #22420
Conversation
dymart
changed the title
dymart
force-pushed
the
multi-node
branch
5 times, most recently
from
136ee44
to
477b28f
Compare
jbedard
force-pushed
the
multi-node
branch
22 times, most recently
from
a6d6450
to
a26eb61
Compare
jbedard
changed the title
|
@alan-agius4 there is one TODO left here but I think this is worth landing as-is for now. WDYT? |
jbedard
force-pushed
the
multi-node
branch
2 times, most recently
from
66d9eea
to
148c480
Compare
| # TODO: test on multiple node versions | ||
| # Some tests use specific ports and can not be run concurrently | ||
| # Some tests fail for unknown reasons when run on multiple platforms |
There was a problem hiding this comment.
These tests are probably the most important that should be ran on multiple platforms.
Do you have examples of the failures?
Probably we can investigate this in the near future.
There was a problem hiding this comment.
One reason migth be the devservers all using port 4200 which we should be able to configure to be random. I'll look into that more if this test is important.
There was a problem hiding this comment.
That shouldn’t be hard to do as the CLI does support port 0.
I can take a look at these maybe in a week.
There was a problem hiding this comment.
I am happy to get the setup in before that. Unless @clydin has any reservations.
There was a problem hiding this comment.
I'm going to work on this and push to this PR but you can merge the first commit now if you'd like
jbedard
force-pushed
the
multi-node
branch
5 times, most recently
from
151e462
to
80b19a9
Compare
alan-agius4
added
action: cleanup
target: minor
jbedard
force-pushed
the
multi-node
branch
3 times, most recently
from
f83fb21
to
ba13232
Compare
packages/angular_devkit/build_angular/src/builders/dev-server/index.ts
Outdated
Show resolved
Hide resolved
| @@ -56,6 +56,19 @@ export type DevServerBuilderOutput = DevServerBuildOutput & { | |||
| baseUrl: string; | |||
| }; | |||
|
|
|||
| export async function _defaultOptions( | |||
There was a problem hiding this comment.
I moved this "default logic" into a method so it can be tested directly instead of having to startup a server and then test that started server uses the default. While I guess the test no longer tests the full e2e server-start it avoids any issues with concurrent tests on the same machine wanting to use the same port.
There was a problem hiding this comment.
The method seems redundant as we the CLI supports port 0 which will cause a free port to be assigned.
There was a problem hiding this comment.
All I did was move it into a function though. I don't think I want to change actual behavior here, just put it in a method that can be tested on its own.
There was a problem hiding this comment.
It seems that the reason why this was extracted into a method was to be used in the integration tests. But that effects the code path that the tests takes.
There was a problem hiding this comment.
Yeah, I can revert it if that is important. But that means tests can not run in parallel...
| @@ -148,3 +148,5 @@ try-import .bazelrc.user | |||
| # Enable runfiles even on Windows. | |||
| # Architect resolves output files from data files, and this isn't possible without runfile support. | |||
| test --enable_runfiles | |||
|
|
|||
| build --experimental_remote_merkle_tree_cache | |||
There was a problem hiding this comment.
This caches the analysis of bazel actions better. Because we have exports_directories_only = False (which ts_library requires?) every single file in the node_moduels gets traversed instead of only the package folders which creates a giant tree. This is also the reason for increasing the circleci resource_class.
|
The flakyness of these tests is very odd, and I think a lot of it has nothing to do with this change. Things like |
Strange as I haven’t seen any similar flakes on the main branch recently. |
| expect(result?.success).toBeTrue(); | ||
| expect(getResultPort(result)).toBe('4200'); | ||
| expect(await response?.text()).toContain('<title>'); | ||
| const result = await _defaultOptions({ browserTarget: 'test:build' }); |
There was a problem hiding this comment.
This change bypasses the end user facing logic.
What we can do in this case to valid default port is the validate that the dev-server started on port 4200, if that fails check for the error messages that port 4200 is in use.
There was a problem hiding this comment.
I'll try that so we don't have to disable parallel tests
| @@ -162,6 +162,7 @@ def pkg_npm(name, pkg_deps = [], use_prodmode_output = False, **kwargs): | |||
| exclude_prefixes = [ | |||
| "packages", # Exclude compiled outputs of dependent packages | |||
| ], | |||
| allow_overwrites = True, | |||
There was a problem hiding this comment.
This needed to be added due to an update to aspect_bazel_lib that someone else must have done. I don't fully understand why the build didn't fail until this PR though?
jbedard
force-pushed
the
multi-node
branch
2 times, most recently
from
4bcc915
to
8ce1cc7
Compare
No description provided.