-
Notifications
You must be signed in to change notification settings - Fork 3
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
UPLOAD-1798/gha-playwright #528
base: main
Are you sure you want to change the base?
Conversation
…LOAD-1798/gha-playwright
…laywright version to a specific value; updated tests to be more robust; added playwright tests to tus-upload-server-ci workflow; removed the endpoint field from the form because setting it is unnecessary and possibly destructive
…LOAD-1798/gha-playwright
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
…e docker compose to call install before running the tests
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
1 similar comment
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
…LOAD-1798/gha-playwright
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
|
||
await expect((await uploadPatchResponsePromise).ok()).toBeTruthy() | ||
await expect((await uploadHeadResponsePromise).ok()).toBeTruthy() | ||
|
||
await page.reload(); | ||
// var refreshes = 0; |
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.
temporarily commented these tests out while we work out the new delivery situation, will probably need to make tests read from the delivery configs to know what the location should be
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.
Would it be simpler to wait a certain amount of time (like 5 seconds) after an upload completes, and then check the status page as apposed to refreshing the page? It's more of a "did we deliver the file in time" kind of approach, and that's what the kotlin tests do.
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.
Regarding the new delivery config, you should be able to use the DEX_DELIVERY_CONFIG_FILE
env var and the yml files defined in upload-server/configs/testing
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.
yeah we definitely need to switch to using the config file, but that was outside the scope of this ticket. I had made that fix before the delivery stuff changed and then commented it out when it did.
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.
we need to make a separate ticket to update everything based on the new configs, should make things easier too
|
||
const fileHeaderContainer= page.locator('.file-header-container') | ||
await expect(fileHeaderContainer.getByRole('heading', { level: 1 }).nth(0)).toHaveText(expectedFileName) | ||
await expect(fileHeaderContainer.getByRole('heading', { level: 1 }).nth(1)).toHaveText("Upload Status: Complete") | ||
await expect(fileHeaderContainer).toContainText(`ID: ${uploadId}`) | ||
await expect(fileHeaderContainer).toContainText(`ID: ${uploadId.split('+')[0]}`) |
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.
added this to remove the tus id from the uploadId that happens on s3
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.
Hoping we can avoid this. Ideally, these tests should have no awareness of the storage impl that the service under test is using. Let's have a convo on how we are manipulating the ID at the presentation layer.
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.
Were we not able so resolve that filename tus string issue in S3? Or did Nicole run out of time before leaving? We should make a ticket to fix it if she just wasn't able to get to it in time
await expect(fileDeliveryNcirdContainer.getByRole('heading', { level: 2 })).toHaveText('NCIRD') | ||
await expect(fileDeliveryNcirdContainer.getByRole('heading', { level: 3 })).toHaveText('Delivery Status: SUCCESS') | ||
await expect(fileDeliveryNcirdContainer).toContainText(`Location: uploads/ncird/${uploadId}`) | ||
// const fileDeliveriesContainer = page.locator('.file-deliveries-container'); |
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.
temporarily commented these tests out while we work out the new delivery situation, will probably need to make tests read from the delivery configs to know what the location should be
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.
Can the expected destinations be part of the test case? (again, this is how the kotlin tests do it)
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.
yes, that's what I was trying to do before the delivery stuff changed, and the delivery configs make it every easier to dynamically check for destinations
@@ -16,7 +16,7 @@ test.describe.skip('File Upload and Trace Response Flow', () => { | |||
const password = process.env.SAMS_PASSWORD; | |||
const url = process.env.UPLOAD_URL; | |||
const PS_API_URL = process.env.PS_API_URL; | |||
const fileName = path.resolve(__dirname, '..', '..', 'upload-files', '10KB-test-file'); | |||
const fileName = path.resolve(__dirname, 'test-data', '10KB-test-file'); |
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.
moved the test file into the same directory so it's all contained
UIPort string `env:"UI_PORT, default=:8081"` | ||
// User Interface Configs | ||
UIPort string `env:"UI_PORT, default=8081"` | ||
UIUrl string |
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.
Before the port for the UI had to include the leading :
which wasn't super intuitive, so I created the UIUrl field so that the port could just be the port
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.
If this is the only reason for splitting this config field, maybe we can change the underlying code to just include the :
. Unless there's a need for the service to be aware of the hostname, which I don't see any need for.
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.
the NewServer expects it to be in that format and I was just going to set it there, but it seemed more sticking with the convention to set it in the appconfig because that's where we are creating all the other configs and maybe we will want to give the UI a hostname eventually
@@ -213,6 +219,12 @@ func ParseConfig(ctx context.Context) (AppConfig, error) { | |||
ac.ServerFileEndpointUrl = ac.ServerUrl + ac.TusdHandlerBasePath | |||
ac.ServerInfoEndpointUrl = ac.ServerUrl + ac.TusdHandlerInfoPath | |||
|
|||
if ac.UIPort != "" { |
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.
creating the UIUrl if the port is set, otherwise UIUrl is set to empty string so that the UI won't start up if the UI_PORT is nil
@@ -240,18 +241,19 @@ func GetRouter(uploadUrl string, infoUrl string) *mux.Router { | |||
return | |||
} | |||
|
|||
uploadUrl, err := url.JoinPath(uploadUrl, id) | |||
uploadDestinationUrl, err := url.JoinPath(uploadUrl, id) |
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.
Removed the Endpoint field from the upload form because it wasn't needed, now passing in the uploadUrl that we are using as the endpoint for tus
|
||
await expect((await uploadPatchResponsePromise).ok()).toBeTruthy() | ||
await expect((await uploadHeadResponsePromise).ok()).toBeTruthy() | ||
|
||
await page.reload(); | ||
// var refreshes = 0; |
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.
Would it be simpler to wait a certain amount of time (like 5 seconds) after an upload completes, and then check the status page as apposed to refreshing the page? It's more of a "did we deliver the file in time" kind of approach, and that's what the kotlin tests do.
|
||
const fileHeaderContainer= page.locator('.file-header-container') | ||
await expect(fileHeaderContainer.getByRole('heading', { level: 1 }).nth(0)).toHaveText(expectedFileName) | ||
await expect(fileHeaderContainer.getByRole('heading', { level: 1 }).nth(1)).toHaveText("Upload Status: Complete") | ||
await expect(fileHeaderContainer).toContainText(`ID: ${uploadId}`) | ||
await expect(fileHeaderContainer).toContainText(`ID: ${uploadId.split('+')[0]}`) |
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.
Hoping we can avoid this. Ideally, these tests should have no awareness of the storage impl that the service under test is using. Let's have a convo on how we are manipulating the ID at the presentation layer.
await expect(fileDeliveryNcirdContainer.getByRole('heading', { level: 2 })).toHaveText('NCIRD') | ||
await expect(fileDeliveryNcirdContainer.getByRole('heading', { level: 3 })).toHaveText('Delivery Status: SUCCESS') | ||
await expect(fileDeliveryNcirdContainer).toContainText(`Location: uploads/ncird/${uploadId}`) | ||
// const fileDeliveriesContainer = page.locator('.file-deliveries-container'); |
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.
Can the expected destinations be part of the test case? (again, this is how the kotlin tests do it)
upload-server/Dockerfile
Outdated
@@ -29,7 +29,7 @@ RUN mkdir -p ./configs/local | |||
COPY --from=builder /app/configs/local/deliver.yml ./configs/local/deliver.yml | |||
|
|||
# Create the 'app' user and group | |||
RUN addgroup -S app && adduser -S -G app app | |||
RUN addgroup -S -g 101 app && adduser -S -u 100 -G app app |
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.
What's the purpose of this change? Maybe belongs in another PR
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.
when Candace made her change to the previous part, it didn't seem to want to work for me until I did that. I can back it out and make it's not necessary now
|
||
await expect((await uploadPatchResponsePromise).ok()).toBeTruthy() | ||
await expect((await uploadHeadResponsePromise).ok()).toBeTruthy() | ||
|
||
await page.reload(); | ||
// var refreshes = 0; |
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.
Regarding the new delivery config, you should be able to use the DEX_DELIVERY_CONFIG_FILE
env var and the yml files defined in upload-server/configs/testing
UIPort string `env:"UI_PORT, default=:8081"` | ||
// User Interface Configs | ||
UIPort string `env:"UI_PORT, default=8081"` | ||
UIUrl string |
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.
If this is the only reason for splitting this config field, maybe we can change the underlying code to just include the :
. Unless there's a need for the service to be aware of the hostname, which I don't see any need for.
tests/smoke/playwright/package.json
Outdated
"test": "npx playwright test" | ||
"wait": "./wait-for-it.sh ${UI_URL:-http://localhost:8081}", | ||
"test": "npm run wait; npx playwright test", | ||
"test:docker": "npm install; npm run test" |
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.
I think the wait step is good, and should be explicitly done here and not in the run test script.
Also... for this, can we include a --workers=2 (or 4 maybe?) to have tests run multithreaded? That might speed up the runs. Podman seems to handle it, can our GHA runners do 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.
So scratch this maybe? I see below we have a workers config.
const config: PlaywrightTestConfig = { | ||
// Specify the directory where your tests are located | ||
testDir: "./test", | ||
|
||
// Use this to change the number of browsers/contexts to run in parallel | ||
// Setting this to 1 will run tests serially which can help if you're seeing issues with parallel execution | ||
workers: 1, | ||
// Opt out of parallel tests on CI. | ||
workers: process.env.CI ? 1 : 4, |
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.
Do we have to opt out of parallel tests on CI? Should we? Parallel tests might let things run faster. Does 2 workers work? Have we tried 4 with the GHA runner?
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.
Playwright recommends only use 1 https://playwright.dev/docs/ci
@@ -31,7 +31,7 @@ test.describe('Upload Manifest Page', () => { | |||
|
|||
test.describe('File Upload Page', () => { | |||
test(`Checks accessibliity for the upload page for the dextesting/testevent1 manifest`, async ({ page }) => { | |||
await page.goto(`/manifest?data_stream_id=dextesting&data_stream_route=testevent1`); | |||
await page.goto(`/manifest?data_stream_id=dextesting&data_stream_route=testevent1`, { waitUntil: 'load' }); |
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.
How necessary do we think this is here? For the accessibility tests, I can see where it might be useful because we're not doing some kind of element get that will retry until a timeout is hit.
But here, the next action we do is a getByLabel which will retry until its found or it will fail (which is kind of what we want, right?). This seems heavy to have to do on EVERY page.goto()
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.
I think we could probably get rid of those, that was my first attempt before adding the wait-for-it
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
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.
Looks great!
…LOAD-1798/gha-playwright
Fortify Scan Results🟢 Status: ✅ Passed Summary
✅ No Action RequiredNo vulnerabilities were identified in this scan. Detailed Results📂 Scanned Path(s)upload-server 📊 Detailed Scan Results
|
tus-upload-server-ci.yml
, testing all upload locations