Skip to content

Conversation

@beardhatcode
Copy link
Contributor

Since I've backported #868 I learned more about the codebase, and now I know how to do certain things nicer.

  • I replaced map(encodeURIComponent).join(/) with the already existing encodeFilePath
  • I added a test to check if cy.get('body > .viewer .icon-error').should('not.exist')
  • I made the oddname test fail faster (5 errors ⇒ next files are not tested and the test is set to failing, this saves a couple of hours in the CI)
  • used eslint on the oddname spec and extracted some common tests between testing the public and private UI

@beardhatcode beardhatcode requested a review from skjnldsv May 2, 2021 10:52
@beardhatcode beardhatcode force-pushed the feat/oddname-clean branch from a49f17f to 24607df Compare May 2, 2021 10:53
@skjnldsv
Copy link
Member

skjnldsv commented May 3, 2021

Please rebase :)

@beardhatcode beardhatcode force-pushed the feat/oddname-clean branch from 24607df to 071c3af Compare May 3, 2021 15:27
instead of split map join

- use generateUrl to automatically encode the basename.
- Properly test if the error page is not shown

Signed-off-by: Robbert Gurdeep Singh <[email protected]>
@beardhatcode beardhatcode force-pushed the feat/oddname-clean branch 2 times, most recently from e7e6251 to aa238c1 Compare May 3, 2021 15:38
@beardhatcode
Copy link
Contributor Author

beardhatcode commented May 3, 2021

I'll need tot test this first since the test has been split

Update: https://dashboard.cypress.io/projects/xysa6x/runs/2568/specs (the quick fail still works 😃 )

@skjnldsv skjnldsv added the 2. developing Work in progress label May 3, 2021
@beardhatcode beardhatcode force-pushed the feat/oddname-clean branch from 9b659d3 to 711ce4b Compare May 3, 2021 17:39
@beardhatcode beardhatcode added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 3, 2021
@skjnldsv
Copy link
Member

skjnldsv commented May 4, 2021

Update: dashboard.cypress.io/projects/xysa6x/runs/2568/specs (the quick fail still works )

I think this link is another run, looking at the github action checks, all the tests really passed 🤔
image

Only one is weird
image

I guess we should make sure to only tests *.spec.js

EDIT: this is the last test: https://dashboard.cypress.io/projects/xysa6x/runs/2569/overview

Comment on lines +47 to +51
let failsLeft = 5
Cypress.on('fail', (error, runnable) => {
failsLeft--
throw error // throw error to have test still fail
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I don't know about this one.
It seems like an odd thing to do, does cypress not support quick-fail on its own?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It supports quick-fail in their "pro plan" (see cypress-io/cypress#518 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using https://github.com/javierbrea/cypress-fail-fast is an option, but that would add a dependency that is only used for one thing (and is implemented in 5 + 3 lines of code)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using javierbrea/cypress-fail-fast is an option, but that would add a dependency that is only used for one thing (and is implemented in 5 + 3 lines of code)

Let get this in then

@beardhatcode
Copy link
Contributor Author

beardhatcode commented May 4, 2021

The cypress link above was to demonstrate that the quickfail works. Before, if there was an error with oddname, the test would take hours (more than 6 hours even, maybe less now that it is split up)

This run for example, from when I was porting the naughtlynames patch to stable20: https://github.com/nextcloud/viewer/runs/2474367200?check_suite_focus=true , it took more than 360 minutes to finish, although it was clear that the test had failed after a few minutes.

@skjnldsv
Copy link
Member

skjnldsv commented May 4, 2021

it took more than 360 minutes to finish

Definitely a bug, cypress is usually super fast 🤔

@skjnldsv skjnldsv merged commit 87541a1 into master May 4, 2021
@skjnldsv skjnldsv deleted the feat/oddname-clean branch May 4, 2021 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants