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

[KED-2099] Use the same JSON mock data files for JS+Python end unit/e2e tests #279

Merged
merged 15 commits into from
Oct 23, 2020

Conversation

studioswong
Copy link
Contributor

Description

This PR aims to create a common test dataset that will be used in tests on both the Kedro-viz and Kedro-core projects.

Development notes

The following are introduced in this PR:

  1. Set up of the dataset (test-data.json) to replace the original data set (animals.mock.js) that was used throughout the tests for the FE.The dataset is based on the animals dataset, with the only difference in the id field under nodes, as well as the edges field in hash data format instead of string format. This is needed to allow the same dataset to be used by the Kedro-core tests.

The same data set has also been passed to Kiyo to test with the BE and could be subjected to revisions.

  1. Update mockState to reference the test-data.json file in constructing the mock data.

  2. Update of all tests to replace the usage of the animals dataset to use the new test-data dataset.

  3. Revision of existing tests that references and tests with the id field of nodes to use hash data rather than the string data as setup previously.

I have still kept the animals dataset given it is still used as a sample data set under data-source.js. I have also kept the reference to the animals dataset within data-source.js and its relevant test.

I have also kept the documentation as is with the reference to the animals dataset - @richardwestenra please let me know your thoughts, and we could revert to replacing the animals dataset entirely with the test-data dataset and I could make the relevant changes in data-source.js and the contribution docs.

Many thanks to @921kiyo and @richardwestenra for your help and collaboration on this ticket!

QA notes

N/A

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@studioswong studioswong self-assigned this Oct 15, 2020
@richardwestenra
Copy link
Contributor

@studioswong Pro tip: Instead of adding "WIP" to the PR title, you can open a draft pull request instead!

@studioswong
Copy link
Contributor Author

@studioswong Pro tip: Instead of adding "WIP" to the PR title, you can open a draft pull request instead!

Thanks for the pro tip Richard 👍 ! Yea I originally thought of opening this as a draft PR as well, but on second thought I think it might be more straight forward to just set this up as an actual PR so we can just merge this in directly on obtaining confirmation from Kiyo with the test data file on their side - my attempts to avoid another PR :D ( The WIP is indicative that I might push new changes for the test data file)

Sorry I should've mentioned this in my description! I can also close this and open another draft one if this turns out to be more more upcoming changes than intended ;)

@@ -0,0 +1,168 @@
{
"selected_pipeline": "__default__",
Copy link
Contributor

Choose a reason for hiding this comment

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

I've just removed input.json in #281, and started using animal test data, so you can delete this file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome, thanks Kiyo!

@studioswong studioswong changed the title WIP [KED-2099] Use the same JSON mock data files for JS+Python end unit/e2e tests [KED-2099] Use the same JSON mock data files for JS+Python end unit/e2e tests Oct 19, 2020
Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

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

Nice work! A couple of notes, that I'm happy to pair with you and discuss if you like

@studioswong
Copy link
Contributor Author

As @richardwestenra and I discussed, I will construct a separate PR for changing demo.mock.js into a json file and updating relevant tests after this PR is merged.

@studioswong studioswong merged commit 42ee6b3 into main Oct 23, 2020
@studioswong studioswong deleted the feature/KED-2099-align-mock-files-FE-BE-tests branch October 23, 2020 10:39
@richardwestenra richardwestenra mentioned this pull request Nov 9, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants