Mainpage e2e test #820
Mainpage e2e test #820
Conversation
|
Deployment previews on netlify for branch
|
tests/e2e/specs/app.spec.js
Outdated
| it('should open and render with some buttons', () => { | ||
|
|
||
| it('should open and render with Login button', async () => { | ||
| const navLogin = await $('#nav-login') |
There was a problem hiding this comment.
It might be helpful to keep all of these in the page object class.
There was a problem hiding this comment.
+1 on this, if we store some selectors on the page object class, let's store all of them there.
| }) | ||
|
|
||
| it('should collapse Login and Signup buttons into a dots-button icon when screen width < 600px', async () => { | ||
| await browser.setWindowSize(599, 800) |
There was a problem hiding this comment.
This will cause any subsequent tests to also be run while in the mobile view. You could set a default browser size in beforeEach to prevent this for happening. For example:
beforeEach(async () => {
await browser.setWindowSize(1360, 973)
)}
There was a problem hiding this comment.
+1 on this, which should still be adressed.
There was a problem hiding this comment.
I added this (line 4-7)
tests/e2e/specs/app.spec.js
Outdated
| }) | ||
|
|
||
| it('should have a button linking to Discovery page', async () => { | ||
| App.open() |
There was a problem hiding this comment.
It might help to move this to beforeEach to make each test is a little less dependent on what happened in prior tests. For example:
beforeEach(async () => {
App.open()
})
But you will likely need more of these:
waitForDisplayed({ timeout: 2000 })
tests/e2e/specs/app.spec.js
Outdated
| await browser.waitUntil( | ||
| async () => (await browser.getUrl()).includes('/discovery'), | ||
| { | ||
| timeout: 5000, | ||
| timeoutMsg: 'expected to be redirected to /discovery within 5s' | ||
| } | ||
| ) | ||
|
|
||
| // Assert the new URL | ||
| const discoveryUrl = await browser.getUrl() | ||
| expect(discoveryUrl).toContain('/discovery') |
There was a problem hiding this comment.
I think this can be simplified down to:
await expect(browser).toHaveUrlContaining('/discovery', { wait: 5000 })
src/components/Layout/Foot.vue
Outdated
| @@ -1,5 +1,5 @@ | |||
| <template> | |||
| <footer class="footer"> | |||
| <footer id="footer" class="footer"> | |||
There was a problem hiding this comment.
Suggestion: A good pattern I've seen used elsewhere is using data-test-id attributes instead of id proper which reduces the risk of inadvertent side effects (this now creates window.footer) and also makes it very clear what the attribute is about, i.e.
<footer data-test-id="footer" class="footer">and then lateron you select the element using $('[data-test-id="footer"]')
This comment also applies to all other places where ids are introduced.
There was a problem hiding this comment.
good idea. I also think that it could be troublesome...
docker-compose.yml
Outdated
| ports: | ||
| - 8081:8081 | ||
| healthcheck: | ||
| test: curl --fail http://ui:8081 || exit 1 |
There was a problem hiding this comment.
Question: Is || exit 1 needed when curl --fail is used? I would think this already exits non-zero on failure, so there's no need to repeat this.
| - uses: actions/checkout@v4.1.6 | ||
| - run: cp .env.development.prod .env | ||
| - run: sudo chown -R 1000:1000 ${GITHUB_WORKSPACE} | ||
| - run: docker compose run ui npm install |
There was a problem hiding this comment.
| - run: docker compose run ui npm install | |
| - run: docker compose run --rm ui npm install |
No need to keep the dangling container post-install.
| }) | ||
|
|
||
| it('should collapse Login and Signup buttons into a dots-button icon when screen width < 600px', async () => { | ||
| await browser.setWindowSize(599, 800) |
There was a problem hiding this comment.
+1 on this, which should still be adressed.
tests/e2e/specs/app.spec.js
Outdated
| it('should open and render with some buttons', () => { | ||
|
|
||
| it('should open and render with Login button', async () => { | ||
| const navLogin = await $('#nav-login') |
There was a problem hiding this comment.
+1 on this, if we store some selectors on the page object class, let's store all of them there.
|
|
||
| jobs: | ||
| browser-tests: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Question: should we pin the ubuntu version here, so we don't get surprise updates at some point in the future?
There was a problem hiding this comment.
which version you suggested? Or I just use the current one because it works?
There was a problem hiding this comment.
It seems 24.04 is the latest one available https://github.com/actions/runner-images
| @@ -28,7 +28,7 @@ | |||
| </v-menu> | |||
| <template v-if="($vuetify.breakpoint.width >= 600) && !isLoggedIn"> | |||
| <v-btn | |||
| id="nav-login" | |||
| data-test-id="nav-login" | |||
There was a problem hiding this comment.
While it's good to use the data-test-ids for testing, I'm not sure it's safe to remove existing id attributes. We'd better leave them in here so code that expects them to exist (in case it exists?) keeps finding them.
No description provided.