-
Notifications
You must be signed in to change notification settings - Fork 114
Testify/playwright #1552
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
base: master
Are you sure you want to change the base?
Testify/playwright #1552
Changes from all commits
9e42dde
43c2d8f
444d280
e780704
a345275
c5842f5
cbf837b
2ac346d
1f0563b
4969fb6
a3b7898
8234fc3
158e195
42c8bc7
e666fb6
b8dc0a3
188b96a
cd595a8
ce12797
b8faf64
21290a0
0a9d352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,12 +14,15 @@ object BlackBoxUtils { | |
| private const val GENERATED_FOLDER_NAME = "generated" | ||
|
|
||
| const val baseLocationForJavaScript = "$JS_BASE_PATH/$GENERATED_FOLDER_NAME" | ||
| const val baseLocationForPlaywright = "$JS_BASE_PATH/$GENERATED_FOLDER_NAME/playwright" | ||
| const val baseLocationForPython = "$PY_BASE_PATH/$GENERATED_FOLDER_NAME" | ||
| const val baseLocationForJava = "$MAVEN_BASE_PATH/src/test/java" | ||
| const val baseLocationForKotlin = "$MAVEN_BASE_PATH/src/test/kotlin" | ||
|
|
||
| fun relativePath(folderName: String) = "$GENERATED_FOLDER_NAME/$folderName" | ||
|
|
||
| fun relativePathPlaywright(folderName: String) = "$GENERATED_FOLDER_NAME/playwright/$folderName" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see previous comment. this function might not be needed
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing relativePathPlaywright in new commit ref previous comment |
||
|
|
||
| fun checkCoveredTargets(targetLabels: Collection<String>) { | ||
| targetLabels.forEach { | ||
| assertTrue(CoveredTargets.isCovered(it), "Target '$it' is not covered") | ||
|
|
@@ -36,7 +39,6 @@ object BlackBoxUtils { | |
|
|
||
| private fun mvn() = if (isWindows()) "mvn.cmd" else "mvn" | ||
|
|
||
|
|
||
| private fun runNpmInstall() { | ||
| val command = listOf(npm(), "ci") | ||
|
|
||
|
|
@@ -120,6 +122,20 @@ object BlackBoxUtils { | |
| runTestsCommand(command, JS_BASE_PATH, "NPM") | ||
| } | ||
|
|
||
| fun runPlaywrightTests(folderRelativePath: String) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need this function and not using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. npx playwright test is the recommended command (ref Playwright documentation), e.g. when using this command there is no need for a script in package.json. However, I've refactored runNpmTests to include Plawyright, and included a playwright script in package.json. Tests locally look promising, so will commit and verify in CI. |
||
| runNpmInstall() | ||
|
|
||
| val path = if(folderRelativePath.endsWith("/")){ | ||
| folderRelativePath | ||
| } else { | ||
| "$folderRelativePath/" | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code here seems duplicated from
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment |
||
|
|
||
| val npx = if (isWindows()) "npx.cmd" else "npx" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is removed in new commit due to refactoring of runNpmTests. |
||
| val command = listOf(npx, "playwright", "test", path) | ||
| runTestsCommand(command, JS_BASE_PATH, "Playwright") | ||
| } | ||
|
|
||
| fun runPythonTests(folderRelativePath: String) { | ||
| installPythonRequirements() | ||
|
|
||
|
|
@@ -151,4 +167,4 @@ object BlackBoxUtils { | |
|
|
||
| fun getOutputFilePrefixKotlin(outputFolderName: String) = "com.kotlin.$outputFolderName.EM" | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,7 @@ abstract class SpringTestBase : GraphQLTestBase() { | |
| lambda: Consumer<MutableList<String>> | ||
| ){ | ||
| val baseLocation = when { | ||
| outputFormat.isPlaywright() -> BlackBoxUtils.baseLocationForPlaywright | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should avoid switches that are order dependent. for example, currently all formats that satisfy note that in future we might have palywright tests that are not related to JS
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In new commit outputFormat.isJavaScript() has been refactored to include Playwright so that baseLocationForJavaScript will be used for both. |
||
| outputFormat.isJavaScript() -> BlackBoxUtils.baseLocationForJavaScript | ||
| outputFormat.isPython() -> BlackBoxUtils.baseLocationForPython | ||
| else -> throw IllegalArgumentException("Not supported output type $outputFormat") | ||
|
|
@@ -100,6 +101,7 @@ abstract class SpringTestBase : GraphQLTestBase() { | |
| fun runGeneratedTests(outputFormat: OutputFormat, outputFolderName: String){ | ||
|
|
||
| when{ | ||
| outputFormat.isPlaywright() -> BlackBoxUtils.runPlaywrightTests(BlackBoxUtils.relativePathPlaywright(outputFolderName)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see previous comment
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New commit includes outputFormat.isPlaywright() still, but without runPlaywrightTests and relativePathPlaywright as these have been removed ref previous comments. Testing locally indicates slightly different handling is required for Playwright, i.e. isPlaywright = true' |
||
| outputFormat.isJavaScript() -> BlackBoxUtils.runNpmTests(BlackBoxUtils.relativePath(outputFolderName)) | ||
| outputFormat.isPython() -> BlackBoxUtils.runPythonTests(BlackBoxUtils.relativePath(outputFolderName)) | ||
| else -> throw IllegalArgumentException("Not supported output type $outputFormat") | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| const { defineConfig } = require('@playwright/test'); | ||
|
|
||
| module.exports = defineConfig({ | ||
| testDir: './generated', | ||
| testMatch: /.*[tT]est\.js/, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,7 +150,6 @@ abstract class SpringTestBase : RestTestBase() { | |
| } | ||
| } | ||
|
|
||
|
|
||
| fun runBlackBoxEM( | ||
| outputFormat: OutputFormat, | ||
| outputFolderName: String, | ||
|
|
@@ -160,6 +159,7 @@ abstract class SpringTestBase : RestTestBase() { | |
| lambda: Consumer<MutableList<String>> | ||
| ){ | ||
| val baseLocation = when { | ||
| outputFormat.isPlaywright() -> BlackBoxUtils.baseLocationForPlaywright | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see previous comments on use of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In new commit outputFormat.isJavaScript() has been refactored to include Playwright so that baseLocationForJavaScript will be used for both. |
||
| outputFormat.isJavaScript() -> BlackBoxUtils.baseLocationForJavaScript | ||
| outputFormat.isPython() -> BlackBoxUtils.baseLocationForPython | ||
| outputFormat.isJava() -> BlackBoxUtils.baseLocationForJava | ||
|
|
@@ -172,6 +172,7 @@ abstract class SpringTestBase : RestTestBase() { | |
| fun runGeneratedTests(outputFormat: OutputFormat, outputFolderName: String){ | ||
|
|
||
| when{ | ||
| outputFormat.isPlaywright() -> BlackBoxUtils.runPlaywrightTests(BlackBoxUtils.relativePathPlaywright(outputFolderName)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see previous comments on use of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New commit includes outputFormat.isPlaywright() still, but without runPlaywrightTests and relativePathPlaywright as these have been removed ref previous comments. Testing locally indicates slightly different handling is required for Playwright, i.e. isPlaywright = true' |
||
| outputFormat.isJavaScript() -> BlackBoxUtils.runNpmTests(BlackBoxUtils.relativePath(outputFolderName)) | ||
| outputFormat.isPython() -> BlackBoxUtils.runPythonTests(BlackBoxUtils.relativePath(outputFolderName)) | ||
| outputFormat.isJava() -> BlackBoxUtils.runJavaTests(outputFolderName) | ||
|
|
@@ -230,4 +231,4 @@ abstract class SpringTestBase : RestTestBase() { | |
| } | ||
| } | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ | |
| <dependency> | ||
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> | ||
| <scope>provided</scope> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is something that was automatically added (by Maven?) when I initially set up the project. My bad for including it in commit, removed in new commit. |
||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
|
|
@@ -133,6 +134,16 @@ | |
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.14.1</version> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this? also, it does not seem related to this PR's goals. if such change was needed for any reason, should had gone in its own separated PR.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment re lombok |
||
| <configuration> | ||
| <annotationProcessorPaths> | ||
| <path> | ||
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> | ||
| <version>1.18.30</version> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why hardcoding here a version of lombok that might diverge from the one declared for the dependency?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment re lombok |
||
| </path> | ||
| </annotationProcessorPaths> | ||
| </configuration> | ||
| </plugin> | ||
| </plugins> | ||
| </build> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ enum class OutputFormat { | |
| KOTLIN_JUNIT_4, | ||
| KOTLIN_JUNIT_5, | ||
| JS_JEST, | ||
| JS_JEST_PLAYWRIGHT, // Testing new playwright imp | ||
| //CSHARP_XUNIT, //no longer supported, but there is still legacy code not removed | ||
| PYTHON_UNITTEST | ||
| ; | ||
|
|
@@ -28,6 +29,12 @@ enum class OutputFormat { | |
|
|
||
| fun isJavaScript() = this.name.startsWith("js_", true) | ||
|
|
||
| /** | ||
| * Return true if the output format is Playwright. | ||
| * Playwright is currently only supported for JavaScript (or TypeScript). | ||
| */ | ||
| fun isPlaywright() = this.name.endsWith("_playwright", true) // Testing new playwright imp | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is comment |
||
|
|
||
| fun isJavaOrKotlin() = isJava() || isKotlin() | ||
|
|
||
| fun isJUnit5() = this.name.endsWith("junit_5", true) | ||
|
|
@@ -40,5 +47,4 @@ enum class OutputFormat { | |
| fun isCsharp() = this.name.startsWith("csharp",ignoreCase = true) | ||
|
|
||
| fun isPython() = this.name.startsWith("python_", true) | ||
|
|
||
| } | ||
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.
why the need for a new folder location instead of using same
baseLocationForJavaScript? also, if really need new folder, should had written like:const val baseLocationForPlaywright = "$baseLocationForJavaScript/playwright"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 isolate the playwright tests and avoid overwriting other tests. But if that's not required, I'll remove it. Or use your suggestion.
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.
@mkugelstad thx. that shouldn't be necessary, as anyway we do a
FileUtils.deleteDirectory(folder.toFile())inSpringTestBasebefore each test case executionThere 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.
Removing baseLocationForPlaywright in new commit