Skip to content

Comments

first Contribute#13

Merged
CatarinaGamboa merged 10 commits intoliquid-java:mainfrom
kiranraoboinapally:main
Apr 25, 2025
Merged

first Contribute#13
CatarinaGamboa merged 10 commits intoliquid-java:mainfrom
kiranraoboinapally:main

Conversation

@kiranraoboinapally
Copy link
Contributor

Cleaning Duplicates in AppTest.java

  • Repeated try-catch blocks

  • Repeated App.launcher("...") calls

  • Repeated assertTrue(e instanceof LatteException)

  • Redundant logging/stack traces

Copy link
Collaborator

@CatarinaGamboa CatarinaGamboa left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @kiranraoboinapally !
The issue #7 about the refactoring was definitely related to the "copy pasted" code that you have removed.
When running mvn test, we should still run about 22 tests, did you test this?

@kiranraoboinapally
Copy link
Contributor Author

kiranraoboinapally commented Apr 18, 2025

Thanks for the contribution @kiranraoboinapally !
The issue #7 about the refactoring was definitely related to the "copy pasted" code that you have removed.
When running mvn test, we should still run about 22 tests, did you test this?

I tried it now but it's checking only 10 test cases in that one pass remaining all are causing failure error let me try to fix it

Copy link
Collaborator

@CatarinaGamboa CatarinaGamboa left a comment

Choose a reason for hiding this comment

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

I am not sure, what is the strategy in this commit, can you clarify that for me?
I looks like this is the initial version

@kiranraoboinapally
Copy link
Contributor Author

for inCorrect examples i maintained same code but for correct examples i made try-catch in a simple and smaller way reducing the tests from 14 to 1 and remaining are incorrect examples test .
I have a doubt where to discuss?

@CatarinaGamboa
Copy link
Collaborator

We can discuss more over here or in the issue page :)

Some points that we need to consider:

  1. Does it make sense to turn all the separate tests into one big test? What if only one of the tests fails? We do not know which one it is, so it is harder to pinpoint the error. We want JUnit tests that are small and effective at catching possible bugs in our code, and give us a direct place to look for the issues.
  2. Have you checked @ParameterizedTest in JUnit? They can do this very cool thing where most of the code is the same for different tests, but some parts differ. We could use it for the Strings of filenames and even error messages. Check it out! Here is a link - https://www.baeldung.com/parameterized-tests-junit-5

Also, to merge any branch into main, we need to resolve the conflicts in the PR first, and we can merge/rebase after. So, first, pull from the main (with rebase preferably) and resolve the conflicts before we merge.
If you want, you can start a new branch in this repo for the approach with @ParameterizedTest if you don't want to handle the conflicts here (since that approach might be a bit different).

@kiranraoboinapally
Copy link
Contributor Author

kiranraoboinapally commented Apr 21, 2025

We can discuss more over here or in the issue page :)

Some points that we need to consider:

1. Does it make sense to turn all the separate tests into one big test? What if only one of the tests fails? We do not know which one it is, so it is harder to pinpoint the error. We want JUnit tests that are small and effective at catching possible bugs in our code, and give us a direct place to look for the issues.

2. Have you checked `@ParameterizedTest` in JUnit? They can do this very cool thing where most of the code is the same for different tests, but some parts differ. We could use it for the Strings of filenames and even error messages. Check it out! Here is a link - https://www.baeldung.com/parameterized-tests-junit-5

Also, to merge any branch into main, we need to resolve the conflicts in the PR first, and we can merge/rebase after. So, first, pull from the main (with rebase preferably) and resolve the conflicts before we merge. If you want, you can start a new branch in this repo for the approach with @ParameterizedTest if you don't want to handle the conflicts here (since that approach might be a bit different).

Thanks for the guide link,if you have any more let me know

@kiranraoboinapally
Copy link
Contributor Author

Changes:

  • Parameterized Tests with Arguments: Used Stream<Arguments> and @MethodSource to pass test data (file paths, expected outcomes, and error messages) to the tests, improving test scalability and reducing redundancy.

  • File Existence Check: Added validation to ensure test files exist before execution.

  • Exception Handling: Simplified exception assertions using assertThrows and assertDoesNotThrow.


!Help: I dont know am going in the right way or not Let me know ?Trying to learn and Implement

Copy link
Collaborator

@CatarinaGamboa CatarinaGamboa left a comment

Choose a reason for hiding this comment

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

This is a great step into the right track 💪
Look at the comments I've left in the code for AppTest.java.
We also need to change the pom file to include the dependencies of the parameterized test - that guide i linked to previously also has that part.
To test that the tests work you can do:

mvn install
mvn test-compile

@CatarinaGamboa
Copy link
Collaborator

@kiranraoboinapally let me know when you want me to review this again

@kiranraoboinapally
Copy link
Contributor Author

kiranraoboinapally commented Apr 22, 2025

Required anymore modifications
Let me know ,If not I will send review request @CatarinaGamboa

Copy link
Collaborator

@CatarinaGamboa CatarinaGamboa left a comment

Choose a reason for hiding this comment

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

This looks great! Can you add javadoc to the functions? (/** ... */)
Did you test with mvn test?
I switched to this branch and it still was not working properly, here is a screenshot of what i got:
Screenshot 2025-04-23 at 3 53 49 PM

@kiranraoboinapally
Copy link
Contributor Author

kiranraoboinapally commented Apr 24, 2025

Build Failure Tests Run:23,Failures:8,Errors:0,skipped:0

I have runned it in Eclipse by opening pom.xml file next I right clicked and there is run as -->maven test

Screenshot (66)

Note: I didnt push currently pom.xml file updated code for above Build failure

And mvn test-compile Build Success

@CatarinaGamboa CatarinaGamboa self-requested a review April 25, 2025 12:15
@CatarinaGamboa CatarinaGamboa merged commit ce49bed into liquid-java:main Apr 25, 2025
1 check passed
@CatarinaGamboa
Copy link
Collaborator

This PR highlighted a BUG in App.java as the incorrect tests were not properly executed! 🔥🚀
This happened because main was just printing and sending the JSON error message, so the program does not crash when connected to the VS Code plugin. This way, it did not produce an exception that was expected by the tests, as shown in your previous test results.
There are 3 tests that do not have the correct results, but I'll take care of it as an issue in a separate PR.

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.

2 participants