Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/include coverage html assets #42

Merged
merged 14 commits into from
Feb 9, 2018
Merged

Feature/include coverage html assets #42

merged 14 commits into from
Feb 9, 2018

Conversation

pesse
Copy link
Member

@pesse pesse commented Jan 30, 2018

Also some refactoring of the tests and maven setup

@pesse
Copy link
Member Author

pesse commented Feb 2, 2018

Aw, just saw the old test don't run. Will have to rewrite them. You can still review the other changes :)

@jgebal
Copy link
Member

jgebal commented Feb 3, 2018 •
edited
Loading

Closes #39
Closes utPLSQL/utPLSQL#518

@pesse
Copy link
Member Author

pesse commented Feb 3, 2018

This PR does not yet solve the original problem. Still need to rewrite cli so it does in fact copy the assets to the output location and set the path for the assets.

@jgebal
Copy link
Member

jgebal commented Feb 4, 2018

I would prefer to have the api download the resources as a dependency at build time from: https://github.com/utPLSQL/utPLSQL-coverage-html
We can make releases there, to have versioning and then include the resources as library in a certain version.
If we go with the proposed solution, it will lead to maintenance issues and risk of having different versions of resource files for the same report.

Pazus
Pazus reviewed Feb 4, 2018
View reviewed changes
src/test/java/org/utplsql/api/FileMapperIT.java Outdated
import org.utplsql.api.rules.DatabaseRule;
import org.junit.Assert;
import org.junit.Rule;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like dependency from jUnit; there are no rules in Jupiter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually a dependency from junit-jupiter-migrationsupport which is the recommended approach by JUnit 5 as long as you can't completely get rid of Rules - which I can't at that point. I see this as topic for a future refactoring.

src/test/java/org/utplsql/api/ReporterNameTest.java
}

@Test
public void reporterFactory() {
Assert.assertTrue(ReporterFactory.createReporter(CustomTypes.UT_COVERAGE_HTML_REPORTER)
assertTrue(ReporterFactory.createReporter(CustomTypes.UT_COVERAGE_HTML_REPORTER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be implemented using harmcrest marchers, which provide a better feedback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about including hamcrest but didn't see the big benefit. Hamcrest is another library, another dependency which is increasing complexity and for the moment I didn't see a reason to introduce it.

src/test/java/org/utplsql/api/TestRunnerIT.java Outdated
@@ -1,8 +1,7 @@
package org.utplsql.api;

import org.junit.Assert;
import org.junit.Rule;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another rule

src/test/java/org/utplsql/api/TestRunnerIT.java Outdated
@@ -25,7 +26,7 @@ public void runWithDefaultParameters() {
Connection conn = db.newConnection();
new TestRunner().run(conn);
} catch (SQLException e) {
Assert.fail(e.getMessage());
fail(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to re-fail just declare throws SQLException and remove try

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will change that

src/test/java/org/utplsql/api/TestRunnerIT.java Outdated
@@ -79,12 +80,12 @@ public void failOnErrors() {
new TestRunner()
.failOnErrors(true)
.run(conn);
Assert.fail();
fail("Test expected to throw exception");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jUnit5 introduced assertThrows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have a look

@pesse
Copy link
Member Author

pesse commented Feb 4, 2018

@jgebal I agree with you in general that dynamic insertion would be nice here, but I wouldn't want to do it now. It will be quite a bit of work because it increases complexity by a good part. We'll have to write tests post-packaging because we compose the behaviour in that step.
I would rather like to go with the current approach and open an issue for future improvement and go on with fixing existing issues now.
What are your thoughts about it?

@jgebal
Copy link
Member

jgebal commented Feb 4, 2018 •
edited
Loading

I was thinking of integration this as part of maven build.
You could download the resources before the build is executed by using this kind of syntax:

<plugin>
  <groupId>com.googlecode.maven-download-plugin</groupId>
  <artifactId>download-maven-plugin</artifactId>
  <version>1.3.0</version>
  <executions>
    <execution>
      <!-- the wget goal actually binds itself to this phase by default -->
      <phase>process-resources</phase>
      <goals>
        <goal>wget</goal>
      </goals>
      <configuration>
        <url>https://codeload.github.com/utPLSQL/utPLSQL-coverage-html/zip/1.0.0</url>
        <outputFileName>utPLSQL-coverage-html</outputFileName>
        <!-- default target location, just to demonstrate the parameter -->
        <outputDirectory>${project.build.resources[0].directory}/CoverageHTMLReporter</outputDirectory>
      </configuration>
    </execution>
  </executions>
</plugin>

source: https://stackoverflow.com/questions/2741806/maven-downloading-files-from-url

And then we can unzip it with:
https://maven.apache.org/plugins/maven-dependency-plugin/examples/unpacking-artifacts.html

I'm not sure if that's all it takes, but would be worth a try.

@jgebal
Copy link
Member

jgebal commented Feb 4, 2018 •
edited
Loading

I got the download part to work, just didn't get a chance to try the unzip part.
The content of release zip from utPLSQL/utPLSQL-coverage-html can be controlled by .gitattributes file.

jgebal
jgebal approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@jgebal jgebal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice set of improvements.
Thanks @pesse

@pesse
Copy link
Member Author

pesse commented Feb 7, 2018

Just noticed a Bug in asset retrieval when packed as jar. Will fix it before merging

@pesse pesse merged commit 548515b into develop Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pazus Pazus Pazus left review comments

@jgebal jgebal jgebal approved these changes

@viniciusam viniciusam Awaiting requested review from viniciusam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pesse @jgebal @Pazus