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

Moved saved_state/compareGenerateSql.. directory from java source to resources bundle #5424

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

MalloD12
Copy link
Contributor

@MalloD12 MalloD12 commented Dec 28, 2023

As discussed recently to avoid confusion on people we have decided to move saved_state/compareGeneratedSqlWithExpectedSqlForMinimalChangesets SQL files from the java source directory and place them all in the resources bundle.

Fixes #5335

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Things to be aware of

  • None

Things to worry about

  • None

Additional Context

@MalloD12
Copy link
Contributor Author

@tati-qalified - @rursprung Guys when you have a chance, could you please try pulling this branch and then running either mvn clean install or mvn test and then run git status to see whether you are getting all these files untracked. After moving them to the resources bundle I don't see them changed after running any of these mvn commands.

Thanks,
Daniel.

Copy link
Collaborator

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

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

Hey @MalloD12 - don't you need to change the path at AbstractVerifyTest.java:39 to point to this new path?

@MalloD12
Copy link
Contributor Author

MalloD12 commented Jan 2, 2024

Hey @MalloD12 - don't you need to change the path at AbstractVerifyTest.java:39 to point to this new path?

Yeap, I think you are right @filipelautert. I didn't update it because all tests were successfully passing, but it makes total sense to update that path. I'll do it and make sure all tests keep passing.

Thanks,
Daniel.

@MalloD12
Copy link
Contributor Author

MalloD12 commented Jan 2, 2024

Hey @MalloD12 - don't you need to change the path at AbstractVerifyTest.java:39 to point to this new path?

Done!

@tati-qalified
Copy link
Contributor

@MalloD12 tested - seems to work as expected!
Here's the output of running mvn test and git status:
image

@rursprung
Copy link
Contributor

i guess in this case i don't need to try this as well since it should reproduce the same way for all of us? :)

@filipelautert filipelautert added this to the 1NEXT milestone Jan 15, 2024
@filipelautert filipelautert merged commit 6db1e5b into master Jan 15, 2024
35 checks passed
@filipelautert filipelautert deleted the fix-issue-5335 branch January 15, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
5 participants