feat(#1417): Test API sketch.

[volodya-lombrozo]

I've implemented a sketch for the future Test API. Notice how much we reduce the test code by encapsulating all properties, parameters, preparations, etc.
It's only the first and an approximate implementation.
@yegor256 @mximp provide your ideas, please.

Sketch for: #1417

[yegor256]

@volodya-lombrozo I would get rid of the interface here. It's test code, let's try to make it as short as possible. Verbosity is a sin in Java :)

[mximp]

@volodya-lombrozo How would I execute ParseMojo, OptimizeMojo and TranspileMojo in sequence with their props configured (see TranspileMojoTest.recompileIfModified() test)?

[yegor256]

@volodya-lombrozo @mximp as suggested before, I would go for this:

@Test
void testExample(@TempDir temp){
  assertThat(
    new FakeMaven()
      .home(temp.resolve("src"))
      .saveDefault()
      .with("cache", "./cache/parsed")
      .execute(RegisterMojo.class)
      .with("anotherParam", true)
      .execute(AssembleMojo.class),
    hasKey("./target/eo-foreign.json")
  );
}

I think, we can handle this with just one class FakeMaven. Don't see why we need any other classes or interfaces. Maybe I'm missing a bigger problem.

[mximp]

@volodya-lombrozo @mximp as suggested before, I would go for this:

I think, we can handle this with just one class FakeMaven. Don't see why we need any other classes or interfaces. Maybe I'm missing a bigger problem.

In this case I agree: since we don't need a hierarchy here - single class would be enough.

[mximp]

@volodya-lombrozo @mximp as suggested before, I would go for this:

@Test
void testExample(@TempDir temp){
  assertThat(
    new FakeMaven()
      .home(temp.resolve("src"))
      .saveDefault()
      .with("cache", "./cache/parsed")
      .execute(RegisterMojo.class)
      .with("anotherParam", true)
      .execute(AssembleMojo.class),
    hasKey("./target/eo-foreign.json")
  );
}

@yegor256 @volodya-lombrozo Maybe we can allow Object as property values?

.with("cache", Paths.of("./cache/parsed"))

[yegor256]

@mximp yes, definitely, I would defined it as with(String, Object)

[volodya-lombrozo]

@volodya-lombrozo I would get rid of the interface here. It's test code, let's try to make it as short as possible. Verbosity is a sin in Java :)
@yegor256 Yea, it was definitely a verbosity, I've already removed it.

[yegor256]

@volodya-lombrozo it seems like we are doing here what Hamcrest matchers must do. I think it's better to stay with standard matchers for a while. Then, when we see that certain matchers are used very often, we will introduce our own. Making FakeMaven do the assertion is a mix of concepts in one place, I would say (a violation of SRP, if you wish).

[volodya-lombrozo]

@yegor256
Well... I can remove it, I see that it definitely looks like overdesign, at lest of now, but I tried to avoid violation of SRP exactly by using CompilationResult class. Also, I wanna hide complexity from programmers eyes. Just compare:

  MatcherAssert.assertThat(
            new Home(target).exists(
                Paths.get(
                    String.format("%s/foo/x/main.%s", ParseMojo.DIR, TranspileMojo.EXT)
                )
            ),
            Matchers.is(true)
        );
        MatcherAssert.assertThat(
            new TjSmart(
                Catalogs.INSTANCE.make(foreign)
            ).getById("foo.x.main").exists("xmir"),
            Matchers.is(true)
        );

with:

MatcherAssert.assertThat(result, new XmirCreatedWell()));

looks beautiful, isn't?

Moreover, In some checks we have to know where target dir is , where eo-foreign.json is placed and etc. In other words, I need to know internal parts of FakeMaven. What should I do? (Write a getter? :) By using CompilationResult I can encapsulate all required internals.

[volodya-lombrozo]

@volodya-lombrozo How would I execute ParseMojo, OptimizeMojo and TranspileMojo in sequence with their props configured (see TranspileMojoTest.recompileIfModified() test)?

It's a good question, actually :) like that:

 new FakeMaven(temp)
                .program("+package f\n\n[args] > main\n  (stdout \"Hello!\").print\n")
                .execute(ParseMojo.class)
                .execute(OptimizeMojo.class)
                .with("transpiledFormat", "csv")
                .execute(TranspileMojo.class)

Actually, if you take a look more precisely, you may notice the next facts:

1. No need to change properties between executions they are the same for all of them. If you need to change them between executions, probably you are doing something wrong.
2. In most cases properties the same between all the tests, only in rare cases we have to add some property or override the existing one. And we will do it using the .with("cache", Paths.of("./cache/parsed")) statement of course.

By that reasons, I can conclude that we can have a simple map with default values, that we will apply for all executions. In rare cases we will use with in order to override some of the properties.

[yegor256]

@volodya-lombrozo maybe you can make .program(String...) -- it will concat all lines into a program

[mximp]

@volodya-lombrozo maybe you can make .program(String...) -- it will concat all lines into a program

Killer feature 👍🏻 :)

[volodya-lombrozo]

@yegor256 @mximp I've removed redundant classes and added the killer feature (.program(String...)). But, I'm still afraid of several things:

1. Verbosity of Matchers:

 MatcherAssert.assertThat(
            new Home(maven.targetPath()).exists(
                Paths.get(
                    String.format("%s/foo/x/main.%s", ParseMojo.DIR, TranspileMojo.EXT)
                )
            ),
            Matchers.is(true)
        );
        MatcherAssert.assertThat(
            new TjSmart(
                Catalogs.INSTANCE.make(maven.foreignPath())
            ).getById("foo.x.main").exists("xmir"),
            Matchers.is(true)
        );

The same code appears several times (at least in ParseMojoTest).

2. Violation of encapsulation.
   I have to use the next methods in order to open internals of FakeMaven:

    /**
     * Path to compilation target directory.
     * @return Path to target dir.
     */
    public Path targetPath() {
        return this.workspace.absolute(Paths.get("target"));
    }

    /**
     * Path to 'eo-foreign.csv' or 'eo-foreign.json' file after all changes.
     * @return Path to eo-foreign.* file.
     */
    public Path foreignPath() {
        return this.workspace.absolute(FakeMaven.FOREIGN_PATH);
    }

[mximp]

@volodya-lombrozo what about suggestion above regarding .with() method for props?

[mximp]

@volodya-lombrozo I believe it's better to create new issue for "future" plan. No need to have it in JavaDoc.

[volodya-lombrozo]

@mximp Sure. Removed.

[yegor256]

@volodya-lombrozo why not do what I suggested earlier: make FakeMaven.exec() return a HashMap<Path, String> with all files created in the directory and their relative names? Then, we can assert on this hash map.

[volodya-lombrozo]

@mximp as for with() - I've added an example how to use it. Please check.
@yegor256 as for HashMap - I've created a puzzle, in order to keep that PR in some borders. Please check.

[yegor256]

@volodya-lombrozo can't you just rename this method to withTojo (or something similar) and get rid of this supplementary class?

[volodya-lombrozo]

@yegor256 fixed. with -> withTojoAttribute

[yegor256]

@volodya-lombrozo to be honest, this looks wrong. Ctors must be code-free. When you have a need to put some code into ctor, your design is in trouble. Here, I would suggest to make a new method FakeMaven.withDefaults(). The user must call it if they need defaults.

[volodya-lombrozo]

@yegor256 fixed.

[yegor256]

@volodya-lombrozo this also looks wrong to me. Let's make our users call this. Now it's happening behind the scene and it's misleading.

[volodya-lombrozo]

@yegor256 Done.

---

But I disagree, because you expose implementation (again, it's about encapsulation):

1. afaik, all tests will require eo-foreign.csv. What is the point to call it each time in each test the same way?
2. You are getting attached to the specific implementation through eo-foreign file, what if we decide to use a different approach? It will difficult to fix all that tests.

[volodya-lombrozo]

@yegor256 please, check

[yegor256]

@volodya-lombrozo why this is called forProgram while other methods start with with? Maybe withProgram would be more logical?

[volodya-lombrozo]

@yegor256 done

[yegor256]

@volodya-lombrozo I would make withProgram(String...) method and then withFile(String, String) method (the first argument is the path of the file and the second argument is the content). Then, withProgram can use withFile and you can get rid of static constants.

[volodya-lombrozo]

@yegor256 done

[yegor256]

@volodya-lombrozo static constants is a bad idea in general, unless you really mean something very "static". In this case, try to use supplementary methods, as I suggested above with withFile and withProgram

[volodya-lombrozo]

@yegor256 done

[yegor256]

@volodya-lombrozo it's a single class FakeMaven. Why do we need a package for it?

[volodya-lombrozo]

@yegor256 done

[volodya-lombrozo]

@yegor256 check, please

[yegor256]

@volodya-lombrozo you use these constants only once in the code, why do you make them as static literals? In general, try to stay away from static literals: https://www.yegor256.com/2015/07/06/public-static-literals.html

[volodya-lombrozo]

@yegor256 fixed.

[volodya-lombrozo]

@yegor256 please, check

[yegor256]

@volodya-lombrozo don't you think that it would be better to add this line to the catalog when we call withProgram? and then, get rid of this this.prog attribute at all. in general, mutable attributes is a very strong signal that the design is wrong.

[volodya-lombrozo]

@yegor256 yea, you right. It's much better. Thank you. Fixed.

[volodya-lombrozo]

@yegor256 Please, check.

[yegor256]

@rultor merge

[rultor]

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

[rultor]

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 20min)