-
-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for file
/files
in JVM tests
#14537
Support for file
/files
in JVM tests
#14537
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
file
/files/
relocated_files` in JVM testsfile
/files
/relocated_files
in JVM tests
|
file
/files
/relocated_files
in JVM testsfile
/files
in JVM tests
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
I've marked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
) -> FallibleClasspathEntry: | ||
|
||
return FallibleClasspathEntry( | ||
"Empty classpath for no-op classpath target", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make for more useful debug logging, this description should probably still include str(request.component)
.
@maybe_skip_jdk_test | ||
def test_vintage_file_dependency(rule_runner: RuleRunner) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to extract a test helper across these three methods... maybe something that takes the BUILD
file content and the final filename to expect as arguments...?
assert re.search(r"1 tests found", test_result.stdout) is not None | ||
|
||
|
||
@pytest.mark.skip # TODO(14537) `relocated_files` doesn't presently work, un-skip when fixing that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.skip # TODO(14537) `relocated_files` doesn't presently work, un-skip when fixing that. | |
@pytest.mark.skip("TODO(14537) `relocated_files` doesn't presently work, un-skip when fixing that.") |
Hm, yea: inspecting this, I don't see any reason why it shouldn't already be working. But given the further work that will be happening with codegen (and it being slightly off topic) deferring it makes good sense.
JdkEnvironment, JdkRequest, JdkRequest.from_field(request.field_set.jdk_version) | ||
jdk, dependencies = await MultiGet( | ||
Get(JdkEnvironment, JdkRequest, JdkRequest.from_field(request.field_set.jdk_version)), | ||
Get(Targets, DependenciesRequest(request.field_set.dependencies)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(applies to both implementations)
One thing to think about here is whether this should be for direct or transitive dependencies. In v1
for JVM it was transitive, which has the advantage of being able to declare test helpers in other targets with their own files
target dependencies (a test helper using a yaml
file, for example). I know less about the semantics of Go though, and whether it would be useful (or even possible) to be transitive in that case cc @Eric-Arellano , @tdyas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Perhaps we land the version with direct dependencies and return to the topic of transitive dependencies later?
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This is largely based on the `golang` approach. Presently `relocated_files` support is broken (at least in tests, it's not immediately clear why) Closes pantsbuild#14524
This is largely based on the
golang
approach, and most of the code here is tests, for some reason :)Closes #14524