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

Update data sources #3042

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Update data sources #3042

merged 1 commit into from
Aug 22, 2024

Conversation

ElizabethOkerio
Copy link
Contributor

@ElizabethOkerio ElizabethOkerio commented Aug 20, 2024

Issues

This pull request fixes #xxx.

Description

This PR updates the common data sources. It adds logic for resetting the data source and creating a copy of the data source by each test to avoid data corruption in cases where tests run in parallel,

It is important to note that xunit tests in different classes run in parallel. This therefore means that if they are updating the same data source then we'll run into data corruption issues which necessitated the tests in 7.x to use TestPriority in running the tests.

To fix this, I've added an action for the different data sources that creates a copy of the original data. This means that each test class will implement this action. The implementation will then be included in the test class's constructor. This will mean that each test will work on a new copy of data every other time and we won't need to use the TestPriority attribute that we were using before. This will also make it easy to add assertions to assert that what we are testing is correct. Most of the tests that we are porting didn't have the assertions as the data was corrupted as it was not being reset correctly.

For creating copies, I'm using System.Text.Json library which does deep copies as the source data has some deep nesting. The other options for copying these data were a bit manual, others depended on reflection hence why I decided to use this library.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@ElizabethOkerio ElizabethOkerio force-pushed the update_action_overloading_tests branch from a116827 to 844cd1e Compare August 21, 2024 06:07
@ElizabethOkerio ElizabethOkerio changed the title Update ActionOverloadingTests update data sources Aug 21, 2024
@ElizabethOkerio ElizabethOkerio changed the title update data sources Update data sources Aug 21, 2024
@ElizabethOkerio ElizabethOkerio force-pushed the update_action_overloading_tests branch 2 times, most recently from 9c3850d to 346d559 Compare August 22, 2024 06:57
marabooy
marabooy previously approved these changes Aug 22, 2024
Copy link
Member

@marabooy marabooy left a comment

Choose a reason for hiding this comment

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

LGTM

@ElizabethOkerio ElizabethOkerio force-pushed the update_action_overloading_tests branch from 412d3d8 to 68ed901 Compare August 22, 2024 12:00
@ElizabethOkerio ElizabethOkerio merged commit ac84fc9 into main Aug 22, 2024
5 checks passed
@ElizabethOkerio ElizabethOkerio deleted the update_action_overloading_tests branch August 22, 2024 14:01
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