Update RpmTest to Junit5 #36

HDouss · 2020-03-12T09:30:42Z

Resolving issue #32

Converting to JUnit 5
Extracting method for common behavior
Rid of PMD warning suppression
Update puzzle for remaining tasks

paulodamaso · 2020-03-12T11:48:33Z

@g4s8 PLease assign me

HDouss · 2020-03-12T15:27:18Z

@0crat status

paulodamaso

@HDouss There are some comments, please take a look

paulodamaso · 2020-03-12T12:50:24Z

src/test/java/com/artipie/rpm/RpmTest.java

-        home.toFile().mkdirs();
-        final Storage storage = new FileStorage(home);
-        final Path bin = this.folder.newFile("x.rpm").toPath();
+    public void updateAarchRpm(@TempDir final Path folder, @TempDir final Path store)


@HDouss Typo in Aarch?

@paulodamaso I don't think so. Please see #32 and #30

paulodamaso · 2020-03-12T12:52:59Z

src/test/java/com/artipie/rpm/RpmTest.java

-            StandardCopyOption.REPLACE_EXISTING
-        );
-        storage.save(new Key.From(key), new RxFile(bin).flow()).get();
+        final Storage storage = RpmTest.save(folder, store, key);


@HDouss Couldn't you inline all in line 110?

@paulodamaso Didn't get it. Inline what?

@HDouss Something like this:

MatcherAssert.assertThat( new Threads<>( threads, new Repeated<Scalar<Boolean>>( threads, () -> { new Rpm(RpmTest.save(folder, store, key)).update(key).blockingAwait(); return true; } ) ), Matchers.iterableWithSize(threads) );

Also, use IsIterableWithSize instead Matchers.iterableWithSize

@paulodamaso Inlined rpm, but I can't inline storage as it is used afterwards.

@paulodamaso could not inline rpm either as it will create a new rpm for each thread. That will make the test fail.

@HDouss Good point. Let it be, then

paulodamaso · 2020-03-12T17:01:45Z

src/test/java/com/artipie/rpm/RpmTest.java

                    String.format("repodata/%s", file)
                ).toFile().exists(),
                Matchers.equalTo(true)
            );
        }
    }
+
+    private static Storage save(final Path folder, final Path store, final String key)


@HDouss I 'dont like the idea of sharing this piece of code among all our test cases; a bug in here could get us false results from all our tests.

@paulodamaso This part is asked by the puzzle. Besides, isn't that the benefit of behavior encapsulation/method extraction? If a bug is there, either a fix will fix all the tests, or the tests should be splitted because they don't require the same behavior anymore. Given that they share the same behavior for now, it is good to have that I think.

@HDouss I prefer to have all code for creating the data needed in my test cases inside the test, but it is a particular preference. I consider that sharing a untested code (usually these helper function in tests aren't tested, since they are not production code) is more harmful than repeating very similar code for the creation of each test data. By the way, having to create lots of complex objects so we can run the tests may point to the fact that we have architectural problems in code, too.

@paulidamaso I understand even I don't fully agree. What do we do with the fact that this is what asked by the puzzle/issue?

@HDouss Don't change anything; I can fill a bug if I feel the need of refactoring it

0crat · 2020-03-12T17:04:25Z

This pull request #36 is assigned to @paulodamaso /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8 /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job

0crat · 2020-03-12T17:22:10Z

@0crat status (here)

@HDouss This is what I know about this job in CT2E6TK9B, as in §32:

The job #36 is in scope for 3hr
The role is REV
The job is assigned to @paulodamaso /z for 3hr
There is a monetary reward attached
@paulodamaso /z will get 25% of their hourly rate for this job
The job doesn't have any impediments
The budget is 15 minutes/points
These users are banned and won't be assigned:
- @HDouss /z: This user reported the ticket
Job footprint (restricted area)

paulodamaso

@HDouss Please take a look at the answers. Also, please add some meaningful test to the PR title.

paulodamaso · 2020-03-12T17:36:23Z

src/test/java/com/artipie/rpm/RpmTest.java

                    String.format("repodata/%s", file)
                ).toFile().exists(),
                Matchers.equalTo(true)
            );
        }
    }
+
+    private static Storage save(final Path folder, final Path store, final String key)


@HDouss I prefer to have all code for creating the data needed in my test cases inside the test, but it is a particular preference. I consider that sharing a untested code (usually these helper function in tests aren't tested, since they are not production code) is more harmful than repeating very similar code for the creation of each test data. By the way, having to create lots of complex objects so we can run the tests may point to the fact that we have architectural problems in code, too.

paulodamaso · 2020-03-12T17:36:31Z

src/test/java/com/artipie/rpm/RpmTest.java

-            StandardCopyOption.REPLACE_EXISTING
-        );
-        storage.save(new Key.From(key), new RxFile(bin).flow()).get();
+        final Storage storage = RpmTest.save(folder, store, key);


@HDouss Something like this:

MatcherAssert.assertThat( new Threads<>( threads, new Repeated<Scalar<Boolean>>( threads, () -> { new Rpm(RpmTest.save(folder, store, key)).update(key).blockingAwait(); return true; } ) ), Matchers.iterableWithSize(threads) );

Also, use IsIterableWithSize instead Matchers.iterableWithSize

0crat · 2020-03-12T17:39:40Z

Job #36 is already in scope

HDouss · 2020-03-12T18:31:02Z

@paulodamaso Fixed. About the last point: the method is just copying a file a store it in the storage. The key/file resource being "dynamic" as it is passed as arg. Besides, this is what have been asked by the issue.

paulodamaso · 2020-03-12T18:49:18Z

@HDouss Just need to merge changes form master now

HDouss · 2020-03-12T19:06:03Z

@paulodamaso Thanks. Done!

paulodamaso · 2020-03-12T19:24:50Z

@g4s8 Good to merge

g4s8 · 2020-03-13T08:09:35Z

@HDouss thanks, merged. Please try to use more sensible PR titles next time.

HDouss · 2020-03-13T08:40:13Z

@HDouss thanks, merged. Please try to use more sensible PR titles next time.

@g4s8 Sure. Thanks!

0crat · 2020-03-13T11:36:13Z

Job was finished in 18 hours, bonus for fast delivery is possible (see §36)

#32

2072e02

g4s8 assigned paulodamaso Mar 12, 2020

0crat added scope role/DEV labels Mar 12, 2020

paulodamaso suggested changes Mar 12, 2020

View reviewed changes

#417 CR

0eee14e

#32 test fail

74e2774

Merge remote-tracking branch 'upstream/master' into issue#32

a74acb7

g4s8 changed the title ~~#32~~ Update RpmTest to Junit5 Mar 13, 2020

g4s8 merged commit 2e738e2 into artipie:master Mar 13, 2020

0crat removed the scope label Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update RpmTest to Junit5 #36

Update RpmTest to Junit5 #36

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso left a comment

paulodamaso Mar 12, 2020

HDouss Mar 12, 2020

paulodamaso Mar 12, 2020

HDouss Mar 12, 2020

paulodamaso Mar 12, 2020

HDouss Mar 12, 2020

HDouss Mar 12, 2020

paulodamaso Mar 12, 2020

paulodamaso Mar 12, 2020

HDouss Mar 12, 2020

paulodamaso Mar 12, 2020

HDouss Mar 12, 2020

paulodamaso Mar 12, 2020

0crat commented Mar 12, 2020

0crat commented Mar 12, 2020

paulodamaso left a comment

paulodamaso Mar 12, 2020

paulodamaso Mar 12, 2020

0crat commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

g4s8 commented Mar 13, 2020

HDouss commented Mar 13, 2020

0crat commented Mar 13, 2020

Update RpmTest to Junit5 #36

Update RpmTest to Junit5 #36

Conversation

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0crat commented Mar 12, 2020

0crat commented Mar 12, 2020

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0crat commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

HDouss commented Mar 12, 2020

paulodamaso commented Mar 12, 2020

g4s8 commented Mar 13, 2020

HDouss commented Mar 13, 2020

0crat commented Mar 13, 2020