Skip to content
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

Feature/cypress visual regression #1355

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Aug 27, 2020

Closes #1339
1339 was becoming too messy 🙈

@skjnldsv skjnldsv force-pushed the feature/cypress-visual-regression branch from 5e38c46 to b1ea513 Compare August 27, 2020 13:37
@cypress
Copy link

cypress bot commented Aug 27, 2020



Test summary

24 0 0 0


Run details

Project nextcloud-vue
Status Passed
Commit 74fb30b ℹ️
Started Aug 27, 2020 2:36 PM
Ended Aug 27, 2020 2:37 PM
Duration 00:42 💡
OS Linux Ubuntu - 18.04
Browser Electron 83

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@skjnldsv skjnldsv force-pushed the feature/cypress-visual-regression branch 5 times, most recently from eb313cc to da0e860 Compare August 27, 2020 14:27
@skjnldsv skjnldsv force-pushed the feature/cypress-visual-regression branch from da0e860 to 4259585 Compare August 27, 2020 14:33
@skjnldsv skjnldsv marked this pull request as ready for review August 27, 2020 14:33
Comment on lines +25 to +35
// import Vue from 'vue'
import AppSidebar from '../../../../src/components/AppSidebar/AppSidebar.vue'
import ActionButton from '../../../../src/components/ActionButton/ActionButton.vue'

// Server CSS styles
import server from '../../../../styleguide/assets/server.css'
import icons from '../../../../styleguide/assets/icons.css'
import variables from '../../../../styleguide/assets/variables.css'

// Import font so CI has the same
import 'fontsource-roboto'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should find something later on to simplify this whole thing.
A bootsrap.js script to import that does all this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we anyway should be independent of the server styles, I think. Would simplify visual testing and the styleguide build. At the moment we actually need to update the server styles with every release (although it was never done) and cannot be sure that it looks the same on every Nextcloud server version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we anyway should be independent of the server styles, I think

I agree! But for fonts there isn't much choices :)
We could have our own base styling though, like variables.scss or icons.
That way we have a standard across our components to build their styles, that should help us and avoid having to write lots of unneeded small lines 😉

@skjnldsv skjnldsv added the 3. to review Waiting for reviews label Aug 27, 2020
@skjnldsv skjnldsv self-assigned this Aug 27, 2020
@skjnldsv skjnldsv added the feature: testing Related to tests label Aug 27, 2020
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@skjnldsv skjnldsv force-pushed the feature/cypress-visual-regression branch from 4259585 to 27c0c29 Compare August 27, 2020 14:46
Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

The tests pass both on my local machine and on CI, so it is probably fine for the moment.

I am still a bit afraid that running the tests on different machines is a bit fragile and breaks if a developers setup is to different from the CI setup. But I guess we can adjust, if that happens.

@skjnldsv
Copy link
Contributor Author

I am still a bit afraid that running the tests on different machines is a bit fragile and breaks if a developers setup is to different from the CI setup. But I guess we can adjust, if that happens.

The only issue was actually the fonts. We'll have to rely on this for now, but I'm pretty ok with this tbh :)

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 🐘

@skjnldsv
Copy link
Contributor Author

👍 🐘

@ChristophWurst I requested for reviews because I think this is something that should get our attention for existing and future components :)

You ok with our tests https://github.com/nextcloud/nextcloud-vue/blob/feature/cypress-visual-regression/tests/visual/components/AppSidebar/AppSidebar.visual.js ?

@ChristophWurst
Copy link
Contributor

@ChristophWurst I requested for reviews because I think this is something that should get our attention for existing and future components :)

You ok with our tests https://github.com/nextcloud/nextcloud-vue/blob/feature/cypress-visual-regression/tests/visual/components/AppSidebar/AppSidebar.visual.js ?

I figured.

This is good stuff indeed. We just have to use it carefully. You don't want to test too much in this high level, these test tend to be very fragile and expensive (both in times to write, fix and maintain as well as how much time they need on CI).

For logic, covering as many paths as possible and the usual we should stick to isolated unit tests.

It's all about the balance :)

@skjnldsv
Copy link
Contributor Author

It's all about the balance :)

Any ressources to determine the proper balance?
I guess we'll see in the long run when too much testing is too much?

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 27, 2020
@skjnldsv skjnldsv merged commit 91fbe19 into master Aug 27, 2020
@skjnldsv skjnldsv deleted the feature/cypress-visual-regression branch August 27, 2020 16:05
@juliusknorr
Copy link
Contributor

Very nice 👏

Great work @raimund-schluessler

@raimund-schluessler
Copy link
Contributor

I think we don't need such tests for every component. But for components with many props and slots which all change the appearance and layout, we really need them. As #1281 and all the necessary follow-up PRs as #1288 and #1303 showed, AppSidebar is just to complex with to many different combinations of props and slots to modify anything without automatic visual tests. So this PR here should allow us to adjust such components with a lower risk of breaking anything.

@ChristophWurst
Copy link
Contributor

Any ressources to determine the proper balance?

Usually the tests should looks like a pyramid where unit tests are the foundation and UI/pixel tests are the top. Which means you have lots of unit tests, some integration tests, a few visual tests and so on. The higher level it gets, the fewer tests you should have. But this is more of a guideline than a rule, as so many things :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: testing Related to tests high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants