Refactor assert_in_code test to use inline Java #854
Conversation
improve test by using new feature for improving tests speed.
WalkthroughThe test for detecting assertion statements in Java code was refactored to use an inline string for the Java source code instead of relying on an external file. The AST is now built directly from this string, and the previously used Java file was deleted. Changes
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
@KachanovYev thank you for the PR!
Please have a look at my comments.
I also think that the PR title & description can be improved to target the actual task at hand more specifically. There is still a bunch of slow tests, so this PR does not solve the problem.
Recently we had a related conversation, where @yegor256 stated, that using filesystem for test fixtures is not that of a big deal anyway #800 (comment)
| def test_assert_in_code(self): | ||
| file = Path(self.cur_file_dir, 'Book.java') | ||
| ast = AST.build_from_javalang(build_ast(file)) | ||
| ast = AST.from_string(book_content()) |
There was a problem hiding this comment.
Since the moment of writing the related issue, we actually opted out from introducing from_string method towards this pattern:
ast = AST.build_from_javalang(build_ast_from_string(content))
It is likely we do not need introducing an alternative constructor from_string.
| from aibolit.patterns.assert_in_code.assert_in_code import AssertInCode | ||
|
|
||
|
|
||
| def book_content() -> str: |
There was a problem hiding this comment.
I we add this test fixure, then we probably do not need Book.java file anymore.
ok have a 2 labels "bug" and "good title" |
@KachanovYev I suggest that you update the title and the description of your PR to more effectively convey the message on the changes you added. Then you could apply the recommendations from my side related to the way we read Note that this project uses PDD. Thus, it is OK to describe the issue broadly, but then implement only a part of it, while adding puzzles. It is clear that creating atomic issues on the first try is difficult and sometimes impractical. This is exactly the case here - I noticed that the tests use filesystem, reported it. It does not mean that the issue can be closed via a single PR. But you are right - I have to update the issue to reflect that it is not that relevant anymore. |
ivanovmg
left a comment
There was a problem hiding this comment.
@KachanovYev thank you for applying my suggestions! It looks good to me. I suggest that you change the title of the PR, though - ast.from_string is not relevant anymore.
Hi |
@yegor256 it looks good to me. |
|
@yegor256 |
#692
Refactor assert_in_code test: use inline Java code and AST.from_string; remove Book.java file
Summary by CodeRabbit
Refactor
Chores