Skip to content

Conversation

alberto-hernandez
Copy link
Contributor

Improved parametrization of the plugin and standarization of the output files of the reporters.

Copy link
Member

@pesse pesse left a comment

Choose a reason for hiding this comment

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

Great stuff, Alberto, thanks a lot for your first contribution!
Some general things I'd really like to see:

  • travis configuration for continuous integration and testing (you can look for examples in utPLSQL-java-api and utPLSQL-cli projects)
  • Functional tests where you check that output is actually created

I have no experience in maven plugin devlopment, so don't hestitate to discuss request changes I make with me - after all we're all here to learn ;)

@Mojo(name = "test", defaultPhase = LifecyclePhase.TEST)
public class UtPLSQLMojo extends AbstractMojo
{
@Parameter(defaultValue = "jdbc:oracle:thin:@localhost:1521:ut3")
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is a leftover of your internal testing/development? I'd rather have it required than filled with some - most likely - wrong default value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Samuel,
These are the default parameters used by utplsql installer so we reuse them in order to add coherence to the tool.
It is not great effort to change them to use the same parametrization than Java API project.

Copy link
Member

Choose a reason for hiding this comment

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

I think that the default value in this place will not be of much use in real-life scenarios.
At work I never use local DB, at home I use 3 different local DB versions using docker.
In both cases, I'd need to adjust that parameter.
Thinking of team collaboration, the db connection&credentials would probably be placed outside of project-specific pom anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed for the same parameters as the java-api

</testPaths>

<!-- Mandatory Attributes -->
<url>url_of_connection</url>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can use utPLSQL-local profile and ${dbUrl}, ${dbUser}, ${dbPass} variables here as I did in https://github.com/utPLSQL/utPLSQL-java-api/blob/ab8804397366bd8fde66365305250fae68fd6e53/pom.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before. Try to add a full test for the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

{
UtPLSQLMojo myMojo = (UtPLSQLMojo) rule.lookupMojo("test", POM_PATH);
Assert.assertNotNull(myMojo);
myMojo.execute();
Copy link
Member

Choose a reason for hiding this comment

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

You check only execution itself, right? Would be great to test the results, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we are going to develop our internal testing project and published as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Would you like to develop directly in utPLSQL/utPLSQL-maven-plugin project?
There are no active developer for this one at the moment.
Can you join our Slack for utPLSQL (there is java channel)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, no problem.

* @author Alberto Hernández
*
*/
public enum ReporterDefault {
Copy link
Member

Choose a reason for hiding this comment

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

We should think about a different approach here in future. We plan to support custom reporters soon and therefore shouldn't rely on static lists of reporters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of our main problems with the plugin was the understanding the input parameters of the utplsql so we try to enforce and document the differents parameters furthermore maven usually try to offer an standard configuration for the user (convention over configuration).
We understand that is a valid approach that once you have implement new reporter in the utpsql package a new plugin release could be developed to implement this functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Every client/user can create their own reporters (outside of main utPLSQL project).
Even though it's not yet documented that was that main vision and idea behind it.
Once we cleanup reporters structure in utPLSQL project and this gets documented, users can have any reporter they want developed locally (in-house) for their own needs.

I would define reporters similarly to what we have in utPLSQL-cli parameters.
Example:

<reporters> <!--default - <reporter><name>ut_documentation_reporter</name></reporter>-->
  <reporter>
    <name>ut_xunit_reporter</name>
    <output_file>test_results.xml</output_file> <!--no default-->
    <output_to_screen>true</output_to_screen> <!--default - false-->
  </reporter>
</reporters>

In general, you could have local reproters (not part of utPLSQL project)

  • ut_save_to_table_reporter reporter, that doesnt give any output, but save test results to table.
  • ut_minimal_reporter - that just gives 'AWESOME!!!' as an output, when no tests failed.
  • etc.

There are just few rules to reporter creation

  • reporter constructor needs to have zero mandatory parameters.
  • reporter needs to be defined under ut_reproter_base

There will be more coming, once I finalize cleanup and refactoring, but the refactoring will not impact consumers too much.

README.md Outdated
* `ignoreFailure`
* Ignore or continue when a test fail
* Default: `${maven.test.failure.ignore}`
* `colorConsole`
Copy link
Member

Choose a reason for hiding this comment

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

We could get aligned with maven settings and not request users to set this flag explicitly.
https://stackoverflow.com/questions/43638595/how-do-i-remove-colors-from-maven-output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the configuration in order to use the maven standard mechanism but I don't have time to test it.

@Mojo(name = "test", defaultPhase = LifecyclePhase.TEST)
public class UtPLSQLMojo extends AbstractMojo
{
@Parameter(defaultValue = "jdbc:oracle:thin:@localhost:1521:ut3")
Copy link
Member

Choose a reason for hiding this comment

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

I think that the default value in this place will not be of much use in real-life scenarios.
At work I never use local DB, at home I use 3 different local DB versions using docker.
In both cases, I'd need to adjust that parameter.
Thinking of team collaboration, the db connection&credentials would probably be placed outside of project-specific pom anyway.

* @author Alberto Hernández
*
*/
public enum ReporterDefault {
Copy link
Member

Choose a reason for hiding this comment

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

Every client/user can create their own reporters (outside of main utPLSQL project).
Even though it's not yet documented that was that main vision and idea behind it.
Once we cleanup reporters structure in utPLSQL project and this gets documented, users can have any reporter they want developed locally (in-house) for their own needs.

I would define reporters similarly to what we have in utPLSQL-cli parameters.
Example:

<reporters> <!--default - <reporter><name>ut_documentation_reporter</name></reporter>-->
  <reporter>
    <name>ut_xunit_reporter</name>
    <output_file>test_results.xml</output_file> <!--no default-->
    <output_to_screen>true</output_to_screen> <!--default - false-->
  </reporter>
</reporters>

In general, you could have local reproters (not part of utPLSQL project)

  • ut_save_to_table_reporter reporter, that doesnt give any output, but save test results to table.
  • ut_minimal_reporter - that just gives 'AWESOME!!!' as an output, when no tests failed.
  • etc.

There are just few rules to reporter creation

  • reporter constructor needs to have zero mandatory parameters.
  • reporter needs to be defined under ut_reproter_base

There will be more coming, once I finalize cleanup and refactoring, but the refactoring will not impact consumers too much.

{
UtPLSQLMojo myMojo = (UtPLSQLMojo) rule.lookupMojo("test", POM_PATH);
Assert.assertNotNull(myMojo);
myMojo.execute();
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to develop directly in utPLSQL/utPLSQL-maven-plugin project?
There are no active developer for this one at the moment.
Can you join our Slack for utPLSQL (there is java channel)

# utPLSQL-maven-plugin
A maven plugin for running Unit Tests with utPLSQL v3+


Copy link
Member

Choose a reason for hiding this comment

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

Is this change making the maven plugin releasable/usable or is it still a work in progress?
I think we're missing information in the readme about it.
If it's a work-in-progress, can you please add this info at the top of readme, to avoid any confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a fully operative plugin with all requested changes implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have access to the Travis account so it will possible to check the test status.

  - Parametrized Name, OutputFile, Console
* Removed default values for connections and use the same as the Java
API
* Added fully maven test in order to accomplish maven testing
  - The only drawback is that we have the parameter connection inside
the pom test.
* Updated the Readme of project
@jgebal
Copy link
Member

jgebal commented Feb 7, 2018

Have you seen this solution?
https://gist.github.com/nochmu/1f640b01a685f3f83a349de2db8a8882

@alberto-hernandez
Copy link
Contributor Author

Hi Jacek,
This is a good approach for the developers of the utplsql package. The main drackback is that every programmer who use it needs the client installed in his local machine.
The maven plugin solution is more scalable due to you don't need a install or maintain a local version of the client, even. The developer only has to be worried of writing good PL/SQL code and its tests :)

@jgebal
Copy link
Member

jgebal commented Feb 8, 2018

Just as you replied I got to think something similar.
You need client installed wherever you want to run maven goal.
You mean this maven exec config in every project.
It's nice complex from user perspective than just using a plugin.

How do you go about requirements for jdbc driver from Oracle?

Should we mention it in the documentation?
Can we add this as a dependency, so that users can download the driver within their maven process?

@alberto-hernandez
Copy link
Contributor Author

The oracle driver dependency is complex to manage because Oracle do not publish neither public or own maven repository. That said, the driver is a transitive dependency that comes from the java-api project so it makes sense that will be be documented in that package.

@jgebal
Copy link
Member

jgebal commented Feb 8, 2018

Well actually they do publish but it's a private repository so you need to provide credentials.
Have a look at readme on API project.
In enterprise, you can always provide a private nexus repo with driver in it I think.

@jgebal
Copy link
Member

jgebal commented Feb 8, 2018

But I agre it's more complicated than it should be

@alberto-hernandez
Copy link
Contributor Author

The common workaround for this is download the package and installed it in your private maven repository. In my opinion, it is a common solution that programmers are familiar with.
If you consider to add the same disclaimer for this driver it is not a problem.

@jgebal
Copy link
Member

jgebal commented Feb 8, 2018

@alberto-hernandez
All looks great. @pesse and me are only concerned of ownership and maintenance burden.
Will you be keen and willing to take responsibility for code maintenance, at least for some time, while there are intense changes and project is still fresh and evolving.

To be honest, we're short on hands and need a custodian for this part of project.

@alberto-hernandez
Copy link
Contributor Author

At least for this year is not a problem to maintain the plugin.

@jgebal
Copy link
Member

jgebal commented Feb 8, 2018

Can you join our slack at:
http: // utplsql-slack-invite.herokuapp.com
this is the best way to chat to others and discuss on random topics

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.

We will need to add Travis configuration for the project to be able to continuously test it.
I'll take care of the Travis service side variables etc.

<name>utplsql-maven-plugin Maven Plugin</name>

<!-- FIXME change it to the project's website -->
<url>http://maven.apache.org</url>
Copy link
Member

Choose a reason for hiding this comment

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

<url>http://utplsql.org</url>
I might have someone to take care of the website so we will be publishing info on all utPLSQL projects there, not just the main project.

</reporters>
<sources>
<source>
<directory>src/test/resources/scripts/sources</directory>
Copy link
Member

Choose a reason for hiding this comment

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

If we give support for sources (file mapping), we will also need to have:

    -owner="app"
    -regex_expression="pattern"
    -type_mapping="matched_string=TYPE[/matched_string=TYPE]*"
    -owner_subexpression=subexpression_number
    -type_subexpression=subexpression_number
    -name_subexpression=subexpression_number

One thin that needs improvement on utPLSQL side is documentation of those options - currently it's vague and insufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Insted of adding more modification for the plugin will not be more useful release this version as is and open a new one aligned with the last version of the java-api(3.0.5)?

@jgebal jgebal merged commit 8eef742 into utPLSQL:develop Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants