diff --git a/tests/MANUAL_MIGRATION.md b/tests/MANUAL_MIGRATION.md new file mode 100644 index 00000000000..4b03f4234ad --- /dev/null +++ b/tests/MANUAL_MIGRATION.md @@ -0,0 +1,77 @@ +### Basics + +Check the [MIGRATING_TESTS guide](./MIGRATING_TESTS.md) first if you have not already done so. + +When migrating tests use the remote server mode as this will make it much quicker to run tests than having to start/stop +Keycloak when you run the test from the IDE. + +Add `@KeycloakIntegrationTest` to the class. + +Change `import org.junit.Test;` to `import org.junit.jupiter.api.Test;` + +Change `org.junit.Before` to `org.junit.jupiter.api.BeforeEach` + +Remove extends `AbstractKeycloakTest` as the new test framework provides injection of resources needed by the test there +is no need for the `AbstractKeycloakTest` and tests should instead inject what they need. + +One thing your test is most likely going to need is a realm, this is now done with: + +``` +@InjectRealm +ManagedRealm realm; +``` + +With this change, most of the time you do not need to get the `RealmResource` via an admin client. Instead, you can use +`realm.admin()` + +### Changed packages/classes + +| Old | New | +|---------------------------------------------|------------------------------------------------| +| org.junit.Assert | org.junit.jupiter.api.Assertions | +| org.keycloak.testsuite.Assert | org.keycloak.tests.utils.Assert | +| org.junit.Test | org.junit.jupiter.api.Test | +| org.keycloak.testsuite.admin.ApiUtil | org.keycloak.tests.utils.admin.ApiUtil | +| org.keycloak.testsuite.util.AdminEventPaths | org.keycloak.tests.utils.admin.AdminEventPaths | + +### Assertions + +Change `import org.junit.Assert;` to `import org.junit.jupiter.api.Assertions;`, and replace `Assert.` with `Assertions.` throughout. + +If the assert passes a message (for example `Assert.assertEquals("Message", expected, actual)`) the message in `Assertions` +is now the last parameter (for example `Assertions.assertEquals(expected, actual, "Message")`). + +### Admin events + +Admin events are handled slightly differently in the new test framework. + +An example for the old testsuite: + +``` +@Rule +public AssertAdminEvents assertAdminEvents = new AssertAdminEvents(this); + +public void myTest() { + assertAdminEvents.assertEvent(realmId, OperationType.CREATE, ...); +} +``` + +Converted to the new test framework: + +``` +@InjectAdminEvents +public AdminEvents adminEvents; + +public void myTest() { + AdminEventAssertion.assertEvent(adminEvents.poll(), OperationType.CREATE, ...); +} +``` + +Notice that there is no need to pass `realmId` when asserting an event, that is because the `AdminEvents` will only +receive events for the current realm. + +For better readability `AdminEventAssertion` provides a method chaining approach to assert various fields in the event +(the example above could be change to `AdminEventAssertion.assertSuccess(adminEvents.poll()).operationType(OperationType.CREATE)...`). + +There is also improved support for skipping events using skip methods, that allows skipping one event (`.skip()`), +multiple events (`.skip(5)`), or skipping all previous events (`.skipAll()`). diff --git a/tests/MIGRATING_TESTS.md b/tests/MIGRATING_TESTS.md index 753e9083181..220fb9c26e8 100644 --- a/tests/MIGRATING_TESTS.md +++ b/tests/MIGRATING_TESTS.md @@ -1,69 +1,78 @@ -### Basics +# How to migrate tests to the new framework? -When migrating tests use the remote server mode as this will make it much quicker to run tests than having to start/stop +## TLDR + +1. cd into the [`migration-util` module](./migration-util) +2. Use the [migration script](./migration-util/migrate.sh) + ```shell + ./migrate.sh SomeTest + ``` + **The script doesn't work?** Follow the [MANUAL_MIGRATION guide](./MANUAL_MIGRATION.md). +3. Fix the rest of the test class. To speed up the process, use the remote server mode when running the test. +4. Use the [commit-migration script](./migration-util/commit-migration.sh) to commit the changes correctly: + ```shell + ./commit-migration.sh + ``` +5. On the PR on GitHub, review the commit that modifies the files +6. Do not squash the commits when merging the PR + +## Migration process + +Migrating tests involves doing a lot of repetitive tasks. We made some automation tooling in the +[`migration-util` module](./migration-util) to make it less annoying. + +### Migrating test classes + +To migrate a test class, you can use the [migrate script](./migration-util/migrate.sh). + +cd into the [`migration-util` module](./migration-util) and run: +```shell +./migrate.sh SomeTest +``` + +The script accepts one parameter that can be either a class name or an absolute path to the file. By default, look-up +starts from [`testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite`](../testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite). + +When run, the script copies the test class and rewrites or adds common test statements, such as: +- adds the `@KeycloakIntegrationTest` annotation +- changes JUnit 4's `@Before` to JUnit 5's `@BeforeEach` +- updates JUnit 4's assertions with their JUnit 5's counterparts + +And more. + +Besides the printed logs, you can pass the script a diff tool to see the changes made: + +```shell +DIFFTOOL="diff --color=always" ./migrate.sh SomeTest +``` +```shell +DIFFTOOL="meld" ./migrate.sh SomeTest +``` + +If the script fails and throws an exception, you can try to fix it or refer to the +[MANUAL_MIGRATION guide](./MANUAL_MIGRATION.md). + +The migrated test shall be in the same package in +[`tests/base/src/test/java/org/keycloak/tests`](../tests/base/src/test/java/org/keycloak/tests). +When migrating tests, use the remote server mode, as this will make it much quicker to run tests than having to start/stop Keycloak when you run the test from the IDE. -Add `@KeycloakIntegrationTest` to the class. +### Committing changes -Change `import org.junit.Test;` to `import org.junit.jupiter.api.Test;` +Migrating some tests requires changing more than 50% of the test class. For this reason, git thinks a new file was +created instead of moved. This causes us to lose the history of the test file. To mitigate this, we have a script to make +all commits for you. -Remove extends `AbstractKeycloakTest` as the new test framework provides injection of resources needed by the test there -is no need for the `AbstractKeycloakTest` and tests should instead inject what they need. - -One thing your test is most likely going to need is a realm, this is now done with: - -``` -@InjectRealm -ManagedRealm realm; +In the [`migration-util` module](./migration-util) run: +```shell +./commit-migration.sh ``` -### Changed packages/classes +The script works only with your git staging area. If it detects the same files are marked as deleted and created, it makes +one commit where the files are moved unchanged to the new location. Then, it commits the changes with the rest of +your staging area. You will be prompted to provide a commit message if you do not wish to use the default one. -| Old | New | -|---------------------------------------------|------------------------------------------------| -| org.junit.Assert | org.junit.jupiter.api.Assertions | -| org.junit.Test | org.junit.jupiter.api.Test | -| org.keycloak.testsuite.admin.ApiUtil | org.keycloak.tests.utils.admin.ApiUtil | -| org.keycloak.testsuite.util.AdminEventPaths | org.keycloak.tests.utils.admin.AdminEventPaths | +When you create a PR and check the "Files changed" tab, you will still see them as deleted and created, which makes the +code review hard. Instead, go to the "Commits" tab and **review the commit that modifies the already moved file**. -### Assertions - -Change `import org.junit.Assert;` to `import org.junit.jupiter.api.Assertions;`, and replace `Assert.` with `Assertions.` throughout. - -If the assert passes a message (for example `Assert.assertEquals("Message", expected, actual)`) the message in `Assertions` -is now the last parameter (for example `Assertions.assertEquals(expected, actual, "Message")`). - -### Admin events - -Admin events are handled slightly differently in the new test framework. - -An example for the old testsuite: - -``` -@Rule -public AssertAdminEvents assertAdminEvents = new AssertAdminEvents(this); - -public void myTest() { - assertAdminEvents.assertEvent(realmId, OperationType.CREATE, ...); -} -``` - -Converted to the new test framework: - -``` -@InjectAdminEvents -public AdminEvents adminEvents; - -public void myTest() { - AdminEventAssertion.assertEvent(adminEvents.poll(), OperationType.CREATE, ...); -} -``` - -Notice that there is no need to pass `realmId` when asserting an event, that is because the `AdminEvents` will only -receive events for the current realm. - -For better readability `AdminEventAssertion` provides a method chaining approach to assert various fields in the event -(the example above could be change to `AdminEventAssertion.assertSuccess(adminEvents.poll()).operationType(OperationType.CREATE)...`). - -There is also improved support for skipping events using skip methods, that allows skipping one event (`.skip()`), -multiple events (`.skip(5)`), or skipping all previous events (`.skipAll()`). \ No newline at end of file +**Do not squash the commits** when merging the PR, or the file history will be lost again. diff --git a/tests/MOVING_TESTS.md b/tests/MOVING_TESTS.md deleted file mode 100644 index 55e02dd3645..00000000000 --- a/tests/MOVING_TESTS.md +++ /dev/null @@ -1,21 +0,0 @@ -# Instructions for moving and refactoring tests to the new testsuite - -When moving and refactoring tests from the old testsuite to the new one it might happen that, once the changes are commited, -the git history of the file is lost. This is due to the way that git internally handles file renames. - -In order to preserve the file history, we have come up with a procedure that will work as the following describes. - -Let's assume we are migrating `SampleTest.java` to the new testsuite. - -1. Move the file `SampleTest.java` (without modifying the content) to the new location using any method you like (`mv`, `git mv`, cut and paste, etc.). -2. Commit that movement using a commit message like `Move SampleTest.java to the new testsuite`. -3. Do all the necessary changes and refactors in the file for using the new testing framework. -4. Commit the refactoring using a commit message like `Refactor SampleTest to use the new testing framework`. -5. Push the changes and create a pull request that will contain both commits. - -Once the pull request is created, it might happen that in the `Files changed` tab we see one file deleted -and a new one (with the refactored code) created. This can make difficult the code review, since we don't see the differences -with the previous code. - -For seeing the changes as in a usual pull request go to the `Commits (2)` tab and select the commit that refactors the code. Comments, -reviews and conversations can be added here and will be visible in the rest of the pull request sections. \ No newline at end of file diff --git a/tests/migration-util/src/main/java/org/keycloak/test/migration/CommonStatementsRewrite.java b/tests/migration-util/src/main/java/org/keycloak/test/migration/CommonStatementsRewrite.java new file mode 100644 index 00000000000..bfbb7b74a10 --- /dev/null +++ b/tests/migration-util/src/main/java/org/keycloak/test/migration/CommonStatementsRewrite.java @@ -0,0 +1,23 @@ +package org.keycloak.test.migration; + +import java.util.Map; + +public class CommonStatementsRewrite extends TestRewrite { + + private final Map STATEMENTS = Map.of( + "testRealmResource()", "managedRealm.admin()" + ); + + @Override + public void rewrite() { + for (int i = 0; i < content.size(); i++) { + String l = content.get(i); + for (Map.Entry entry : STATEMENTS.entrySet()) { + if (l.contains(entry.getKey())) { + replaceLine(i, l.replace(entry.getKey(), entry.getValue())); + info(i, "Statement rewritten: '" + entry.getKey() + "' --> '" + entry.getValue() + "'"); + } + } + } + } +} diff --git a/tests/migration-util/src/main/java/org/keycloak/test/migration/MigrateTest.java b/tests/migration-util/src/main/java/org/keycloak/test/migration/MigrateTest.java index bf992c99f3c..e3e870679b8 100644 --- a/tests/migration-util/src/main/java/org/keycloak/test/migration/MigrateTest.java +++ b/tests/migration-util/src/main/java/org/keycloak/test/migration/MigrateTest.java @@ -5,12 +5,18 @@ import java.io.BufferedWriter; import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedList; import java.util.List; import java.util.stream.Stream; +import static java.lang.Thread.sleep; + public class MigrateTest { private static final String DIFF_COMMAND = System.getenv("DIFFTOOL"); @@ -22,7 +28,8 @@ public class MigrateTest { UpdateAssertsRewrite.class, AddManagedResourcesRewrite.class, AdminEventAssertRewrite.class, - BeforeRewrite.class); + BeforeRewrite.class, + CommonStatementsRewrite.class); Path rootPath = getRootPath(); Path oldTestsuitePath = rootPath.resolve("testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite").toAbsolutePath(); @@ -66,9 +73,20 @@ public class MigrateTest { writeFile(content, destinationPath); if (DIFF_COMMAND != null && !DIFF_COMMAND.isEmpty()) { + List args = new ArrayList<>(List.of(DIFF_COMMAND.split(" "))); + args.add(testPath.toString()); + args.add(destinationPath.toString()); + ProcessBuilder pb = new ProcessBuilder(); - pb.command(DIFF_COMMAND, testPath.toString(), destinationPath.toString()); - pb.start(); + pb.command(args); + Process diffProcess = pb.start(); + BufferedReader diffOutput = new BufferedReader(new InputStreamReader(diffProcess.getInputStream())); + String line = diffOutput.readLine(); + while (line != null) { + System.out.println(line); + line = diffOutput.readLine(); + } + diffOutput.close(); } } diff --git a/tests/migration-util/src/main/java/org/keycloak/test/migration/RenameImportsRewrite.java b/tests/migration-util/src/main/java/org/keycloak/test/migration/RenameImportsRewrite.java index a4ec816119c..f6eb42f0c04 100644 --- a/tests/migration-util/src/main/java/org/keycloak/test/migration/RenameImportsRewrite.java +++ b/tests/migration-util/src/main/java/org/keycloak/test/migration/RenameImportsRewrite.java @@ -6,20 +6,37 @@ public class RenameImportsRewrite extends TestRewrite { Map IMPORTS = Map.of( "org.junit.Assert", "org.junit.jupiter.api.Assertions", + "org.keycloak.testsuite.Assert", "org.keycloak.tests.utils.Assert", "org.junit.Test", "org.junit.jupiter.api.Test", "org.keycloak.testsuite.util.AdminEventPaths", "org.keycloak.tests.utils.admin.AdminEventPaths", "org.keycloak.testsuite.admin.ApiUtil", "org.keycloak.testframework.util.ApiUtil" ); + Map STATIC_IMPORTS = Map.of( + "org.junit.Assert", "org.junit.jupiter.api.Assertions" + ); + @Override public void rewrite() { for (int i = 0; i < findClassDeclaration(); i++) { String l = content.get(i); - if (l.startsWith("import ")) { + + if (l.startsWith("import static ")) { + String current = l.substring("import static ".length(), l.lastIndexOf('.')); + String method = l.substring(l.lastIndexOf('.'), l.length() - 1); + String migrateTo = STATIC_IMPORTS.get(current); + + if (migrateTo != null) { + replaceLine(i, l.replace(current, migrateTo)); + + info(i, "Static import rewritten: '" + current + method + "' --> '" + migrateTo + method + "'"); + } + } else if (l.startsWith("import ")) { String current = l.substring("import ".length(), l.length() - 1); String migrateTo = IMPORTS.get(current); + if (migrateTo != null) { - replaceLine(i, "import " + migrateTo + ";"); + replaceLine(i, l.replace(current, migrateTo)); info(i, "Import rewritten: '" + current + "' --> '" + migrateTo + "'"); } diff --git a/tests/migration-util/src/main/java/org/keycloak/test/migration/TestRewrite.java b/tests/migration-util/src/main/java/org/keycloak/test/migration/TestRewrite.java index 6775fc2edbb..bbdfc8d145d 100644 --- a/tests/migration-util/src/main/java/org/keycloak/test/migration/TestRewrite.java +++ b/tests/migration-util/src/main/java/org/keycloak/test/migration/TestRewrite.java @@ -44,14 +44,19 @@ public abstract class TestRewrite { String add = "import " + clazzName + ";"; int l = -1; + int lastImport = -1; for (int i = 0; i < content.size(); i++) { String c = content.get(i); if (c.matches("import [^ ]*;")) { + lastImport = i; if (c.compareTo(add) > 1) { l = i; break; } + } else if (c.matches("^\\b(?:public\\s+)?class\\b.*Test\\s+\\{$")) { + l = lastImport + 1; + break; } } diff --git a/tests/migration-util/src/main/java/org/keycloak/test/migration/UpdateAssertsRewrite.java b/tests/migration-util/src/main/java/org/keycloak/test/migration/UpdateAssertsRewrite.java index c34ad2c6ae0..a2326a16e41 100644 --- a/tests/migration-util/src/main/java/org/keycloak/test/migration/UpdateAssertsRewrite.java +++ b/tests/migration-util/src/main/java/org/keycloak/test/migration/UpdateAssertsRewrite.java @@ -6,29 +6,52 @@ import java.util.List; public class UpdateAssertsRewrite extends TestRewrite { + private final static String FAIL = "fail"; + private final static String ASSERT_THROWS = "assertThrows"; + private final static String ASSERT_TRUE = "assertTrue"; + private final static String ASSERT_FALSE = "assertFalse"; + private final static String ASSERT_NULL = "assertNull"; + private final static String ASSERT_NOT_NULL = "assertNotNull"; + private final static String ASSERT_EQUALS = "assertEquals"; + private final static String ASSERT_NOT_EQUALS = "assertNotEquals"; + @Override public void rewrite() { for (int i = 0; i < content.size(); i++) { String l = content.get(i); String trimmed = l.trim(); - if (trimmed.startsWith("Assert.")) { + if (trimmed.startsWith("Assert.") && trimmed.endsWith(";")) { String method = trimmed.substring("Assert.".length(), trimmed.indexOf("(")); int arguments = l.substring(l.indexOf("(") + 1, l.lastIndexOf(")")).split(", ").length; - if (method.equals("fail")) { - directReplace(i, "fail"); - } else if (method.equals("assertTrue") && arguments == 1) { - directReplace(i, "assertTrue"); - } else if (method.equals("assertNull") && arguments == 1) { - directReplace(i, "assertNull"); - } else if (method.equals("assertNotNull") && arguments == 1) { - directReplace(i, "assertNotNull"); - } else if (method.equals("assertNotNull") && arguments == 2) { - moveMessageToLast(i, "assertNotNull"); - } else if (method.equals("assertEquals") && arguments == 2) { - directReplace(i, "assertEquals"); - } else if (method.equals("assertEquals") && arguments == 3) { - moveMessageToLast(i, "assertEquals"); + if (method.equals(FAIL)) { + directReplace(i, FAIL); + } else if (method.equals(ASSERT_THROWS) && arguments == 3) { + moveMessageToLast(i, ASSERT_THROWS); + } else if (method.equals(ASSERT_TRUE) && arguments == 1) { + directReplace(i, ASSERT_TRUE); + } else if (method.equals(ASSERT_TRUE) && arguments == 2) { + moveMessageToLast(i, ASSERT_TRUE); + } else if (method.equals(ASSERT_FALSE) && arguments == 1) { + directReplace(i, ASSERT_FALSE); + } else if (method.equals(ASSERT_FALSE) && arguments == 2) { + moveMessageToLast(i, ASSERT_FALSE); + } else if (method.equals(ASSERT_NULL) && arguments == 1) { + directReplace(i, ASSERT_NULL); + } else if (method.equals(ASSERT_NULL) && arguments == 2) { + moveMessageToLast(i, ASSERT_NULL); + } else if (method.equals(ASSERT_NOT_NULL) && arguments == 1) { + directReplace(i, ASSERT_NOT_NULL); + } else if (method.equals(ASSERT_NOT_NULL) && arguments == 2) { + moveMessageToLast(i, ASSERT_NOT_NULL); + } else if (method.equals(ASSERT_EQUALS) && arguments == 2) { + directReplace(i, ASSERT_EQUALS); + } else if (method.equals(ASSERT_EQUALS) && arguments == 3) { + moveMessageToLast(i, ASSERT_EQUALS); + } else if (method.equals(ASSERT_NOT_EQUALS) && arguments == 2) { + directReplace(i, ASSERT_NOT_EQUALS); + } else if (method.equals(ASSERT_NOT_EQUALS) && arguments == 3) { + moveMessageToLast(i, ASSERT_NOT_EQUALS); } } }