[#1503] feat(IT): Gravitino web e2e test framework #1689
Conversation
| } | ||
|
|
||
| @Override | ||
| public void downloadWebDriver() { |
There was a problem hiding this comment.
Why aren't these downloads implemented with a separate script?
There was a problem hiding this comment.
I think better use JAVA method to download web browser driver.
| if (!loaded) { | ||
| fail(); | ||
| } | ||
| Dimension d = new Dimension(1920, 1080); |
There was a problem hiding this comment.
The resolution here should not be hardcoded, and 1920*1080 is too large; many monitors are not that big. Let's ask @ch3yne to determine an appropriate size.
There was a problem hiding this comment.
1440*1080 or 1440*900 is usually enough.
| } | ||
|
|
||
| if (!loaded) { | ||
| fail(); |
There was a problem hiding this comment.
Please give a clear error message.
There was a problem hiding this comment.
+1, Please do not use Junit 4 API
| @Override | ||
| public Boolean apply(WebDriver d) { | ||
| String gravitinoVersion = d.findElement(By.id("gravitino_version")).getText(); | ||
| String projectVersion = System.getenv("PROJECT_VERSION"); |
There was a problem hiding this comment.
PROJECT_VERSION should retrieved by gravitino server api
There was a problem hiding this comment.
But I worry Gravitino server failed to start.
There was a problem hiding this comment.
If the Gravitino server failed to start. The tester also failed. So it does't matter
| @@ -269,6 +271,7 @@ tasks.test { | |||
|
|
|||
| // Default use MiniGravitino to run integration tests | |||
| environment("GRAVITINO_ROOT_DIR", rootDir.path) | |||
| environment("IT_PROJECT_DIR", buildDir.path) | |||
There was a problem hiding this comment.
Why do we need this environment? can we refer it from GRAVITINO_ROOT_DIR?
There was a problem hiding this comment.
Avoid splice integration-test module path, and hard code System.getenv("GRAVITINO_ROOT_DIR") + '\' + 'integration-test'
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class AbstractWebIT extends AbstractIT { |
There was a problem hiding this comment.
Can you please add some information about it and how we can use it.
Furthermore, add a description of the content subclass that extends it needs to add.
| this.chromeDriverBinName = ITUtils.joinPath("chromedriver_win32", "chromedriver.exe"); | ||
| this.chromeBinName = ITUtils.joinPath("chrome-win", "chrome.exe"); | ||
| } else { | ||
| throw new RuntimeException("Unsupported OS"); |
There was a problem hiding this comment.
Please add more information about the error message. like SystemUtils.OS_NAME
| String chromeZipFile = "", chromeDriverZipFile = ""; | ||
| if (SystemUtils.IS_OS_LINUX) { | ||
| // https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/Linux_x64%2F1000022%2Fchrome-linux.zip?generation=1651778257041732&alt=media | ||
| chromeZipFile = "chrome-linux.zip"; |
There was a problem hiding this comment.
I think making chromeDownloadURL and chromeDriverDownloadURL a constant value can be better.
There was a problem hiding this comment.
I accepted your suggestion.
| } | ||
|
|
||
| if (!loaded) { | ||
| fail(); |
There was a problem hiding this comment.
+1, Please do not use Junit 4 API
|
@yuqi1129 Please help me review this PR again, Thanks. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
What changes were proposed in this pull request?
Chrome&Chrome Driverinto./intergration-test/build/chromedirectory. need to download about 150 MB zip file for the first time.Why are the changes needed?
Gravitino Web UI needs an E2E test to ensure Web UI usability.
Fix: #1503
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
CI Passed