javascript: Nodejs tests batch support #18742
Conversation
| class JSTestBatchCompatibilityTagField(TestsBatchCompatibilityTagField): | ||
| help = help_text( |
There was a problem hiding this comment.
I sort of think that setting default="default" here would be a sane default, but I also believe that there's value in keeping this as close to 1:1 as the python variant.
There was a problem hiding this comment.
+1 to matching Python's default behavior of no batching. That way we keep fine-grained caching by default.
tobni
force-pushed
the
add/js-test-batching
branch
from
322133b
to
1849bf7
Compare
tobni
changed the title
tobni
changed the title
| @@ -332,3 +333,41 @@ def _get_sources_field_name(field_set_type: type[FieldSet]) -> str: | |||
| + " Pants can't provide a default partitioner." | |||
| ) | |||
| return sources_field_name | |||
|
|
|||
|
|
|||
| class TestsBatchCompatibilityTagField(StringField): | |||
There was a problem hiding this comment.
I don't think this is the right place to define this. Some of our other recurring TestXYZ fields are defined in src/python/pants/core/goals/test.py (i.e. TestTimeoutField, TestExtraEnvVarsField). Could you move it there? And add metaclass=ABCMeta to it so it matches the others?
| class JSTestBatchCompatibilityTagField(TestsBatchCompatibilityTagField): | ||
| help = help_text( |
There was a problem hiding this comment.
+1 to matching Python's default behavior of no batching. That way we keep fine-grained caching by default.
|
|
||
| @property | ||
| def description(self) -> str: | ||
| return f'{self.owning_package.ensure_owner()[NodePackageNameField].value} {self.compatibility_tag or ""}' |
There was a problem hiding this comment.
Maybe it's not showing in the UI, but it looks to me like owning_package is only ever used to invoke ensure_owner(). Could the output of ensure_owner() be the thing that's tracked as a field in the TestMetadata?
| file: str, | ||
| working_directory: str, | ||
| ) -> FilesystemCoverageReport: | ||
| # It is up to the user to configure the output coverage reports. | ||
| file_path = PurePath(file) | ||
| output_dir = nodejs_test.render_coverage_output_dir(dist_dir, address) | ||
| output_dir = nodejs_test.render_coverage_output_dir(dist_dir, addresses[0]) |
There was a problem hiding this comment.
In the Python impl we incorporate batch size into the names of any extra outputs: https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/python/goals/pytest_runner.py#L329
What do you think about doing the same here?
There was a problem hiding this comment.
Currently users name the output files and directories for coverage reports. I think your suggestion would require some sort of interpolation impl? Or do you mean incorporate it in the output directory name?
There was a problem hiding this comment.
I did mean interpolation, though maybe that's not appropriate (I'm not sure what the impl of render_coverage_output_dir currently does). If the address is somehow being incorporated into the outputs, I'd recommend having render_coverage_output_dir take all the addresses as args instead of picking an arbitrary one
There was a problem hiding this comment.
I've added the batch information to the rendered output path.
|
Thanks a lot! Will bow out and let @danxmoran shipit, since he drove Python's test batching. |
tobni
force-pushed
the
add/js-test-batching
branch
from
c1fcfdb
to
2e4c4e8
Compare
Add support for running tests in batches via the nodejs package manager test runners.