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

Improve message-web text format issue #596 #1029

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lezhdz98
Copy link

@lezhdz98 lezhdz98 commented Jan 8, 2025

Hello @therealryan, this is my first PR on Open Source, let me know if this is good or I need to improve something

Thank you!

@therealryan
Copy link
Contributor

Hi @lezhdz98! Welcome, and congratulations on your first PR!

I'm no longer at mastercard, so I'm not in charge of this project any more. You can hit up @vchaitanya for more empowered support (for example, I can't approve the workflow runs).

That said, I've had a quick look at your change and there are a couple of things to fix:

  • It looks like your IDE is applying a different format than the one this project uses, so there are a bunch of whitespace changes that obscure the intent of your changes. You should be able to run mvn formatter:format to apply the project-standard format and hopefully reduce the size of the diff. As it stands the workflow runs (once they are approved) will fail due to non-standard formatting.
  • I'd have expected your diff to include a test change - if you've improved the legibility of multiline values then the expected value in this test will probably have to change. You should be able to run that test class via your IDE. I'd expect it to fail, then you just need to update the expected value so that it passes again.

Cheers!

…, and modified the tests to this new format
@lezhdz98
Copy link
Author

lezhdz98 commented Jan 9, 2025

Hi @vchaitanya, I improve the format for the message-web texts and also modified the tests to be accurate with the new format.
Please let me know if there are some issues with the improvement. Thank you

@vchaitanya
Copy link
Collaborator

Hi @vchaitanya, I improve the format for the message-web texts and also modified the tests to be accurate with the new format. Please let me know if there are some issues with the improvement. Thank you

Hi @lezhdz98, I don't think we need the change in pom.xml. The build seems to be failing due to that change. Can you revert the changes in pom.xml.

@lezhdz98
Copy link
Author

@vchaitanya I reverted the changes in pom.xml :)

@vchaitanya
Copy link
Collaborator

vchaitanya commented Jan 13, 2025

@lezhdz98,
I must have missed this point earlier. You seem to be using java 11 functions, while the project compile version is java 8.
Can you please try making your changes compatible with java 8.

Also please check the failing test ExampleSystemTest.inheritanceHealth

@vchaitanya
Copy link
Collaborator

vchaitanya commented Jan 13, 2025

Hi @lezhdz98,
I've raised a PR #1035 to update the java version of the repo. Your current code should suffice once you take the latest pull.

Regarding the failing test, you can read more about InheritanceHealth. The reason ExampleSystemTest.inheritanceHealth might be failing is due to assertable() method being called within this class. This assertable then calls the asHuman() method which you have updated. The slight change in the format is causing the values to be updated. Updating the expected value should be sufficient.

The Mutation build is also failing. You can check this readme for more details about pitest and mutation. Based on the report, adding/updating a test scenario which covers the missed conditional branch should pass this test. If we have a scenario with the valueWidth greater than "Values".length(), it might provide the necessary coverage.

Let me know if you need help with something.

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