review: fix: Fix the insertion order for CtCaseImpl.insertBegin #4063
Conversation
Rohitesh-Kumar-Jain
changed the title
There was a problem hiding this comment.
Implementation LGTM. The test can be improved.
First of all, like I mentioned in another review, we don't typically test the specific implementations directly, and in fact you're not testing CtCaseImpl directly here. We rely on the factories to create the implementations for us. There is already a test class for cases (called SwitchCaseTest for some reason instead of CtCaseTest, we should rename that). Put the test in there.
Second, the indentation in the test class is incorrect (should be tabs). Make sure that the test case, when moved, has tabs as indentation.
See also the comments on the code.
Co-authored-by: Simon Larsén <slarse@kth.se>
Co-authored-by: Simon Larsén <slarse@kth.se>
Co-authored-by: Simon Larsén <slarse@kth.se>
Co-authored-by: Simon Larsén <slarse@kth.se>
Yeah it should be there, also I will migrate it along with the other test I am working parallel
Yeah, sure IDEs have an auto refactoring feature for I will use that to get the best tab spacing |
| } | ||
| List<CtStatement> copy = new ArrayList<>(statements.getStatements()); | ||
| statements.setStatements(null); | ||
| this.statements.addAll(0, copy); |
There was a problem hiding this comment.
I'm not sure how important this is, but the addStatement(int, CtStatement) method does a little bit more:
a) it calls statement.setParent(this); for each CtStatement
b) it calls onListAdd
I think it is necessary to loop over all statements to add at some point to keep that behavior. I'm not sure if List#addAll(int, Collection) is still prefered then.
@slarse WDYT?
There was a problem hiding this comment.
You're absolutely right. I just had it in my mind that the statement list here was a model list (as it is in CtBlockImpl), but it's not. It's just an ArrayList.
It seems to me like the statement and expression lists in CtCaseImpl should be model lists as well, but I'm not sure about the design decision for them not being so currently. It could simply be that model lists were introduced at a later time, and were never added to CtCaseImpl. I think I'll want to wait for @monperrus or @nharrand to weigh in there before we proceed.
@Rohitesh-Kumar-Jain Can you add assertions to check that the parents of the inserted statements are set correctly? Something like so:
assertThat(firstStatementToBeInserted.getParent(), is(testCase));
assertThat(secondStatementToBeInserted.getParent(), is(testCase));Note the use of identity (is) as opposed to equality.
There was a problem hiding this comment.
Yes I have added these tests as well
I don't get this one, also I ran IntelliJ's auto code reformat, but it failed to detect what are you pointing at ig |
|
@slarse I have implemented all the suggested changes except for this
I am not able to understand this one |
There was a problem hiding this comment.
@Rohitesh-Kumar-Jain Nice fix, looks good to me.
Second, the indentation in the test class is incorrect (should be tabs).
I am not able to understand this one
When you create a new file with IntelliJ, the default is to create it with spaces as indentation. If you look at your first commit, the test class has spaces as indentation. If you just run auto-formatting on that, it will still format with spaces.
When you pasted the test case into the existing test class, IntelliJ detected the use of tabs and changed the indentation automatically.
Thanks for letting me, and I hope you enjoyed your vacations and the Olympics matches : ) |
Vacations: yes. Olympics: I'm just about vaguely aware they exist :) |
WIP
Fix #4060
Also, this test is killing this mutation
link to #3967