Skip to content
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

Java: Port all integration tests to pytest #17315

Merged
merged 7 commits into from
Aug 29, 2024
Merged

Java: Port all integration tests to pytest #17315

merged 7 commits into from
Aug 29, 2024

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented Aug 27, 2024

Some notes:

  • These tests rely on a variety of fixtures
  • The previous maven-wrapper checks were checking for the version of maven installed by looking at the checked-in wrapper script. I dropped this behavior.
  • I replaced a lot of test queries that queried for a (subset of) source archive files with the source_archive fixture. In particular, tests that excluded properties files from being listed in the expected output now include them.
    It's much faster to generate this list via the fixture instead of using a codeql query.

This is also used by the integration tests.
Some notes:
* These tests rely on a variety of fixtures
* The previous maven-wrapper checks were checking for the version of maven installed by looking at the checked-in wrapper script. I dropped this behavior.
* I replaced a lot of test queries that queried for a (subset of) source archive files with the source_archive fixture. In particular, tests that excluded properties files from being listed in the expected output now include them.
   It's much faster to generate this list via the fixture instead of using CodeQL for it.
@criemen criemen added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 27, 2024
@github-actions github-actions bot added the Java label Aug 27, 2024
@criemen criemen marked this pull request as ready for review August 28, 2024 13:31
@criemen criemen requested a review from a team as a code owner August 28, 2024 13:31
redsun82
redsun82 previously approved these changes Aug 29, 2024
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor stylistic nits, otherwise looks good to me!

Comment on lines 5 to 21
commands.run(["javac", "mod1/module-info.java", "mod1/mod1pkg/Mod1Class.java", "-d", "mod1obj"])
commands.run(
[
"jar",
"-c",
"-f",
"mod1.jar",
"-C",
"mod1obj",
"mod1pkg/Mod1Class.class",
"--release",
"9",
"-C",
"mod1obj",
"module-info.class",
]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistic nit, you can use strings instead of lists of strings, and combine the two calls:

Suggested change
commands.run(["javac", "mod1/module-info.java", "mod1/mod1pkg/Mod1Class.java", "-d", "mod1obj"])
commands.run(
[
"jar",
"-c",
"-f",
"mod1.jar",
"-C",
"mod1obj",
"mod1pkg/Mod1Class.class",
"--release",
"9",
"-C",
"mod1obj",
"module-info.class",
]
)
commands.run(
"javac mod1/module-info.java mod1/mod1pkg/Mod1Class.java -d mod1obj",
"jar -c -f mod1.jar -C mod1obj mod1pkg/Mod1Class.class --release 9 -C mod1obj module-info.class",
)

Comment on lines 5 to 21
commands.run(["javac", "mod1/module-info.java", "mod1/mod1pkg/Mod1Class.java", "-d", "mod1obj"])
commands.run(
[
"jar",
"-c",
"-f",
"mod1.jar",
"-C",
"mod1obj",
"mod1pkg/Mod1Class.class",
"--release",
"9",
"-C",
"mod1obj",
"module-info.class",
]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, you can combine the commands.run calls and use one-line strings

@criemen criemen merged commit 72e2910 into main Aug 29, 2024
13 of 16 checks passed
@criemen criemen deleted the criemen/pytest-java branch August 29, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on internal PR This PR should only be merged in sync with an internal Semmle PR Java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants