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

Create Test Data Directory #132

Merged
merged 16 commits into from
Jul 22, 2021
Merged

Create Test Data Directory #132

merged 16 commits into from
Jul 22, 2021

Conversation

mikeisen1
Copy link
Contributor

Finished subtask EGRC-469 where I migrated the data into one directory and updated test
and storybook file imports as necessary.

@kylelaker kylelaker changed the base branch from develop to EGRC-275 July 20, 2021 22:43
@kylelaker
Copy link
Contributor

Congratulations on getting your first PR opened in the oscal-react-library repo! I have re-targeted this PR to have the base branch (the branch that the PR will be merged in to) to the EGRC-275 branch instead of develop. You won't need to change anything locally; this was purely a change on the GitHub side.

There are also a few things that I'd like to make sure we get cleaned up in your local git configuration including the user.name and user.email values and making sure that they're setup correctly for your repository. We can talk about a few other git-related things at the same time.

Love that we are able to move all this stuff into files that can be referenced from various places and deduplicate a lot of this. While everything here look good at first glance, I am going to hold off on approving until I've had a chance to look a little more closely and we've been able to work together on some of the git configuration items.

Previously, all of the data was in various files, even duplicated
in some instances (across not within files). Added a new directory,
test-data, which contains the 7 files where all of the data for
testing and the storybook are located.

When creating the files, removed the duplications of the data by
placing the duplications into one variable. Separated the data by
category for ease of finding the data when needed.
@tuckerzp
Copy link
Contributor

For the linting errors, I believe that @hreineck has had a similar issue in the past. I think the fix was to make sure you update and commit example/package-lock.json & package-lock.json. It is running npm install and updating those files since they are not up to date.

@kylelaker
Copy link
Contributor

@tuckerzp Good catch! That is #135 as well.

Copy link
Contributor

@hreineck hreineck left a comment

Choose a reason for hiding this comment

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

Awesome work, Michael! Have a few small issues to resolve but this is really great overall.

src/test-data/BackMatterData.js Outdated Show resolved Hide resolved
src/stories/OSCALModification.stories.js Show resolved Hide resolved
mikeisen1 and others added 2 commits July 21, 2021 11:18
Co-authored-by: Henry Reineck <43356777+hreineck@users.noreply.github.com>
Co-authored-by: Henry Reineck <43356777+hreineck@users.noreply.github.com>
@mikeisen1
Copy link
Contributor Author

I committed the changes suggested by @hreineck and updated the package-lock.json, but I am still getting the issues of failing the linter check.

@hreineck
Copy link
Contributor

I committed the changes suggested by @hreineck and updated the package-lock.json, but I am still getting the issues of failing the linter check.

Try npm install and then committing package-lock.json

@hreineck
Copy link
Contributor

Actually, looks like it's complaining about example/package-lock.json as well. Maybe try committing the changes to that too.

@tuckerzp
Copy link
Contributor

Try npm install and then committing package-lock.json

I am surprised that fff7cb9 doesn't fix that.

In the code checks for the pull request, there is a linting
error regarding the package-lock JSON files. It states that
local changes to those files would be overwritten by a
checkout and the changes must be committed. The package-lock
JSON files were updated and committed to resolve the linting
errors.
@mikeisen1
Copy link
Contributor Author

mikeisen1 commented Jul 21, 2021

For the lint checks, I was initially having an issue of not being able to checkout another branch even locally. I fixed that issue, ran the npm install command in both directories, and double-checked that the git checkout to another branch would work. That is now working locally but the lint check is still failing. Does anyone have advice on how I can resolve this?

The error message presented when I was not able to locally check out was the exact same as the lint check error message.

hreineck and others added 4 commits July 21, 2021 13:51
Updating package-lock.json and example/package-lock.json
to sync them up with the remote branch.
This should fix the failures in the linting actions.
Update four test files to resolve the linting errors
from the code check of the previous commit.
Modified test files which improperly imported the default
functions, causing the lint check and testing to fail.
Modify two test files to resolve prettier/prettier
style errors, in order to pass the lint check.
Modify two test files to conform with the linter
style requirements, in order to pass the lint
check.
Copy link
Contributor

@hreineck hreineck left a comment

Choose a reason for hiding this comment

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

Awesome job!

@kylelaker
Copy link
Contributor

@mikeisen1 now that we've merged all the other stuff into EGRC-275, can you take one more look at everything to make sure all the data is organized correctly? I think #116 may have added more stuff

mikeisen1 and others added 4 commits July 21, 2021 17:04
Move data over from 4 additional stories that
were added, satisfying the requirement of
having all test and storybook data in one
directory. This makes it easier to use data
across tests and stories, as well as view
the actual data used.
Updating package-lock.json and example/package-lock.json
to sync them up with the remote branch.
This should resolve the linting errors.
Committing changes to package-lock files
in order to fix the failing linter checks.
Copy link
Contributor

@kkennedy26 kkennedy26 left a comment

Choose a reason for hiding this comment

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

Looks good! Ran the tests and storybook, everything is functioning as expected

@kylelaker kylelaker merged commit 301f43e into EGRC-275 Jul 22, 2021
@kylelaker kylelaker deleted the EGRC-469 branch July 22, 2021 16:46
@tuckerzp tuckerzp changed the title EGRC-469 Create Test Data Directory Jul 22, 2021
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.

None yet

5 participants