good first issue: check if assertTrue has a message argument

[monperrus]

@MartinWitt points out rightly that assertTrue assertions should always have a message, see https://github.com/INRIA/spoon/pull/4298#pullrequestreview-814313046

It's a good Spoon exercise to write a processor to detect assertTrue without messages.

We welcome first contributors to Spoon who add missing messages to assertTrue.

[NAMANIND]

Hey 👋🏻, I am new to open source and really excited to do something for community, can you please assign this work to me?

[NAMANIND]

@MartinWitt thank you for assigning this work to me. Can you please explain me exactly what I have to do.

After reading some previous comments, I think I have to add a comment message in the code like this (message: Couldn't find path to maven home). If yes then on which line number or place I should add this comment?

[algomaster99]

Hi @NAMANIND !

First of all, thank you for volunteering to contribute! The issue is about checking if assertTrue has a message parameter also. Note that Junit has overloaded the method name assertTrue with the following definitions:

1. assertTrue(java.lang.String message, boolean condition)
2. assertTrue(boolean condition)

The former one is more desirable because whenever the test fails, JUnit also displays the message passed. This makes it easier for developers to debug what went wrong. In the latter case, only an AssertionError is thrown without any explicit message which is very unclear.

Now, you need to test that for every test method which uses assertTrue, it must follow the first definition above. For example,

1. https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/test/variable/AccessFullyQualifiedFieldTest.java#L66 is the desirable way of using assertTrue.
2. https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L215 is undesirable.

I think I have to add a comment message

To answer your question again, you just have to check if all methods which use assertTrue follows the first definition. You can start by creating a test in spoon which takes all test suites (including itself) as input. After that, you can use Spoon's API to check for the desirable usage of assertTrue like so:

rootPackage.getTypes().forEach(
    ctType -> ctType.getMethods().forEach(
        ctMethod -> ctMethod.getBody().getStatements().forEach(
            ctStatement -> ctStatement.toString().contains("assertTrue"))));

rootPackage refers to this directory src/test/java/spoon (which should you input).

The above code will help you find all the assertTrue assertion statements and then you can check if it has two parameters or not.

[NAMANIND]

Hi @algomaster99 ! After checking all methods in spoon/src/test/java/spoon/MavenLauncherTest.java Which uses assertTrue , I found several assertTrue assertion statement which have no String used which is undesirable.

All of them are mentioned below :-

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L78

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L97

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L146

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L215

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L216

I found several other way of using assertTrue statement too, I don't know that it is right or wrong please give a check on these assertTrue.

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L88

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L107

https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L207

Please see all the above things I got and tell me what to do next.

[algomaster99]

@NAMANIND you don't need to check these test methods manually. Instead, you need to write a test suite to check if each test method follows a contract - 'assertTrue assertion has a message argument too'. You can have a look at this test suite as an example. It checks that each setter of the metamodel invokes setParent and trigger an event change. Although you need to do simpler things than what this test does, but I think you can get an idea of what this issue entails.

I don't know that it is right or wrong please give a check on these assertTrue.

There is nothing right or wrong here. Let's prefer to use desirable and undesirable. The last three test cases which you linked have assertTrue statements with two arguments so it must be the first definition and thus, it falls in the desirable category. Moreover, the tests you will, eventually, write should pass such cases.

[NAMANIND]

Hey @algomaster99 I have wrote a test suite and run all the test cases. Let me explain you with an example what I found :-

See until this line will executed https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L72

We can't reach this line https://github.com/INRIA/spoon/blob/f898632bbdaf5c671b74a652c04d10dfc65328c3/src/test/java/spoon/MavenLauncherTest.java#L78

Thus, How can I add missing messages to assertTrue without knowing that it is true or not because that line is not executed untill above line get execute. Although like in this example I can add a message:"file(cpe) doesn't exist" like something.

Please tell me what to do in it.

[algomaster99]

@NAMANIND

I have wrote a test suite

You should submit a PR for it from your fork and we can start the review process. What do you think?

How can I add missing messages to assertTrue without knowing that it is true

I think the title of the issue may have caused this confusion. You don't have to add a message if it is not present. You just need to check if it is there. If it is not there, the test you design should fail.

@monperrus could you rewrite the issue title as "good first issue: check if assertTrue has a message argument", or something similar along those lines?

[monperrus]

done.

[algomaster99]

Hi @NAMANIND! Did you make any progress? Do you need some help with this issue?

[Salma0104]

Hey, I'm also new to open source and would like to get some experience. I've taken a look at this issue and tried to create a test file using ideas presented in this convo. I'm finding it quite difficult to access the parameters of each statement, but I was able to test if the statements direct children contained "assertTrue(boolean,java.lang.String)" . Any help/guidance would be greatly appreciated.

[SirYwell]

Hey @Salma0104, I'm not sure if I understand how much you got so far. If you already wrote some code, it might be helpful to share it here, that way we can answer more straightforward.

If you're trying to get the arguments from a method invocation, you can access them through the CtAbstractInvocation#getArguments() method.

I'm not sure if that helps, so let me know :)

[Salma0104]

@SirYwell thanks for the reply. After launching and building the model where the resource is the root folder mentioned " src/test/java/spoon" , I wrote the code below. I believe after finding statements that contain assertTrue it gets the direct children and finds whether one of its children is of the type "assertTrue(boolean,java.lang.String)". This test ends up failing because some assertTrue dont contain strings and have just a Boolean. Is this what you wanted, is there another way i could have approached this? Please let me know. model.getAllTypes().forEach( ctType -> ctType.getMethods().forEach( ctMethod -> {if(ctMethod.getBody() != null) { ctMethod.getBody().getStatements().forEach(ctStatement -> { if(ctStatement.toString().contains("assertTrue")) { assertTrue(!ctStatement.getDirectChildren().toString() .contains("assertTrue(boolean,java.lang.String)"));}});}}));

[algomaster99]

Hi @Salma0104 ! Thanks for contributing. Your idea is correct. I have three suggestions:

1. Iterate over type members instead of methods because Junit5 has a feature to create nested classes in test cases. You can use ctType.getTypeMembers() API.
2. Instead of using string comparison to check method signature. You can check that the method's arguments are exactly two and of type String and boolean respectively.
3. Consider submitting a PR. It is much easier to provide feedback. :)

This test ends up failing because some assertTrue dont contain strings and have just a Boolean.

Yes, that is expected. @SirYwell , any ideas on how we can ignore them for now so that we can proceed to write this test case without it failing the CI? I was thinking to add a single line comment to ignore checking for all assertTrue expressions that would fail the test. We can then fix it in the subsequent PRs.

[I-Al-Istannen]

I have another: Use a Processor to find all invocations. This would look like

    launcher.addProcessor(new AbstractProcessor<CtInvocation<?>>() {
      @Override
      public void process(CtInvocation<?> invocation) {
        // check if it is assertTrue without a message
      }
    });

I believe this automatically traverses into inner classes and makes it a bit easier to check :)

---

I'd propose the test to be marked with the meta test-annotation@GithubIssue overhauled in https://github.com/INRIA/spoon/pull/4588. This ensures the test is still run and once it is fixed we can just mark it as such, but we won't forget it - if all violations are fixed and the test no longer fails but is still marked as "shouldFail", the extension breaks the build.

[Salma0104]

Thank you all for replys, im having a bit of trouble understanding the system and classes are there any resources that would help.

[MartinWitt]

Hi, We have the spoon website with pretty good started guides for what is the metamodel, how to query it and advanced topics.