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

Validate mocks - first step #4762

Merged
merged 16 commits into from
May 1, 2020
Merged

Validate mocks - first step #4762

merged 16 commits into from
May 1, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Apr 14, 2020

In addition to heavy cleanup and updating (by applying some non-deprecated attributes e.g. title object), this PR adds jasmine tests for validating all current mocks using Plotly.validate.
The test called mock_test is drafted in commit 8c1a9d9 which currently bypasses undefined layout, invisible traces, coloraxis, dynamic e.g. textposition arrays.
With this relaxation,
before modifying any mocks running the test resulted in
TOTAL: 1014 | FAILED: 487 | SUCCESS: 527
After updating the mocks the situation improved as listed below:
TOTAL: 1014 | FAILED: 174 | SUCCESS: 840.

@alexcjohnson
cc: @plotly/plotly_js

cc: #4733

@nicolaskruchten
Copy link
Contributor

@alexcjohnson let's review this once shape-drawing is done :)

@alexcjohnson
Copy link
Collaborator

@archmoj I just had a quick look at this - very nice! Let's not touch it until 1.54.0 is released, but as this doesn't impact features at all we can include it in a patch after that.

My primary question is about the hard-coded list in mocks_test - seems like we should be able to

  • read in the list of all mocks from the filesystem - perhaps if this step is fast enough we can just have karma.conf.js do it before every test run and write the results to a file in tests/jasmine/assets?
  • only list a set of mocks or patterns to ignore - and then over time we can reduce that to an empty list.

@archmoj archmoj added this to the v1.54.1 milestone Apr 30, 2020
@archmoj
Copy link
Contributor Author

archmoj commented Apr 30, 2020

My primary question is about the hard-coded list in mocks_test - seems like we should be able to

  • read in the list of all mocks from the filesystem - perhaps if this step is fast enough we can just have karma.conf.js do it before every test run and write the results to a file in tests/jasmine/assets?
  • only list a set of mocks or patterns to ignore - and then over time we can reduce that to an empty list.

@alexcjohnson
That is a very good idea we could try in another PR.
At first I did try to adapt our image test for this purpose.
But it was rather complicated to acess the system files from the jasmine test.
I think the way the required mocks are listed is OK atm.
It is actually handy for fixing those many mocks.

@alexcjohnson
Copy link
Collaborator

That is a very good idea we could try in another PR.

🍻 fair enough - added to #4733 so we address it before considering that issue closed.

This looks great. You currently have the require statements for the mocks all commented out - can we enable the ones you've fixed already, just comment out the still-failing ones, and call this PR done?

@archmoj
Copy link
Contributor Author

archmoj commented May 1, 2020

This looks great. You currently have the require statements for the mocks all commented out - can we enable the ones you've fixed already, just comment out the still-failing ones, and call this PR done?

Done in efd8518.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

@archmoj archmoj merged commit 771b9cf into master May 1, 2020
@archmoj archmoj deleted the validate-mocks branch May 1, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants