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.

Sign up for GitHub

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

Jump to bottom

test: migrations file #6819

Merged
merged 10 commits into from
Jul 20, 2023
Merged

test: migrations file #6819

merged 10 commits into from
Jul 20, 2023

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Jul 17, 2023 •
edited
Loading

Description
This is an implementation to test and avoid future migration issues.

This test covers two cases

  • The latest version of migration is updated with the number of migrations
  • The latest migration implemented it's returning the state

Issue

Progresses #6818

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@tommasini tommasini requested a review from a team as a code owner July 17, 2023 12:55
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Gudahtt
Gudahtt previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@MarioAslau
Copy link
Contributor

Thank you for taking the time to jump on a call @tommasini. As I mentioned my only question is in regards to how we will maintain this test file for the future.

The current test is specific to migration 19, but it will not hold up if another migration is introduced that for eg: a test that will need mocking, will result in the test failing.

I don't know if there is a way to make the test general enough to hold up for any new migration that will be introduced.

Do you think we should start adding a new test case of each new migration as we go along in the future instead of re-editing this test?

@tommasini
Copy link
Contributor Author

Yeah, you are right! If we change the state we are actually breaking the test!

It's actually a good idea to have each member write a test if a migration it's implemented!!

Will push that code asap

cc: @Gudahtt What do you think about this approach?

@tommasini tommasini force-pushed the test/migrations-file branch from da3c93e to d18bdd7 Compare July 18, 2023 15:11
MarioAslau
MarioAslau previously approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@MarioAslau MarioAslau left a comment

Choose a reason for hiding this comment

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

Looking good!

Gudahtt
Gudahtt previously approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Makes sense!

@tommasini tommasini dismissed stale reviews from Gudahtt and MarioAslau via a9005ff July 18, 2023 20:51
@tommasini tommasini force-pushed the test/migrations-file branch from fb5b474 to 553f94c Compare July 19, 2023 09:13
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Cal-L
Cal-L reviewed Jul 19, 2023
View reviewed changes
app/store/migrations.test.js Outdated Show resolved Hide resolved
app/store/migrations.test.js Outdated Show resolved Hide resolved
Cal-L
Cal-L approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L added No QA Needed Apply this label when your PR does not need any QA effort. team-mobile-client Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking release-7.5.0 Issue or pull request that will be included in release 7.5.0 and removed No QA Needed Apply this label when your PR does not need any QA effort. labels Jul 19, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@tommasini tommasini merged commit c3b06f4 into main Jul 20, 2023
@tommasini tommasini deleted the test/migrations-file branch July 20, 2023 11:15
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-platform Mobile Platform team label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Cal-L Cal-L Cal-L approved these changes

@Gudahtt Gudahtt Gudahtt left review comments

@MarioAslau MarioAslau MarioAslau left review comments

Assignees
No one assigned
Labels
release-7.5.0 Issue or pull request that will be included in release 7.5.0 Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking team-mobile-platform Mobile Platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tommasini @MarioAslau @Gudahtt @Cal-L @gauthierpetetin