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.

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

[DOXIA-618] import tests from maven-site-plugin #52

Conversation

bertysentry
Copy link
Contributor

@bertysentry
Copy link
Contributor Author

@michael-o @hboutemy This PR is in relation with the removal of some integration tests from maven-site-plugin. These tests have been re-incorporated in Doxia so that we make sure to catch potential problems early. Also, this allows us to implement new features or change the behavior of the parsers without having to modify tests that are too picky in maven-site-plugin (or test that are just become irrelevant).

@elharo
Copy link
Contributor

elharo commented Jan 6, 2021

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@bertysentry
Copy link
Contributor Author

@elharo Yes, this is weird. The build fails... on JDK7 only. To be honest, I don't see what I changed that could possibly break things... on JDK7 and still work in JDK8.

@elharo
Copy link
Contributor

elharo commented Jan 6, 2021

Perhaps try cutting this up into smaller pieces if possible.

@bertysentry
Copy link
Contributor Author

Builds have been failing on JDK7 since this commit: b55285c

There are 2 error messages.

First error:

org.codehaus.plexus.component.repository.exception.ComponentLookupException: 
Unable to lookup component 'org.apache.maven.doxia.parser.Parser', it could not be started.
      role: org.apache.maven.doxia.parser.Parser
  roleHint: markdown
classRealm: plexus.core
-----------------------------------------------------
realm =    plexus.core
strategy = org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy
Number of foreign imports: 0

-----------------------------------------------------

I really don't see what could have triggered this problem. It must be either in MarkdownParser.java or MarkdownParserTest.java

But since I haven't touched the component declaration, I don't understand what is wrong here.

2nd error message

at org.apache.maven.doxia.module.markdown.MarkdownParserTest.setUp(MarkdownParserTest.java:58)
Caused by: org.codehaus.plexus.component.repository.exception.ComponentLifecycleException: Error constructing component role: 'org.apache.maven.doxia.parser.Parser', implementation: 'org.apache.maven.doxia.module.markdown.MarkdownParser', role hint: 'markdown'
	at org.apache.maven.doxia.module.markdown.MarkdownParserTest.setUp(MarkdownParserTest.java:58)
Caused by: java.lang.ExceptionInInitializerError
	at org.apache.maven.doxia.module.markdown.MarkdownParserTest.setUp(MarkdownParserTest.java:58)
Caused by: java.util.regex.PatternSyntaxException: 
Illegal/unsupported escape sequence near index 97
\A^\s*(?:title|author|date|address|affiliation|copyright|email|keywords|language|phone|subtitle)\h*:\h*\V*\h*$\v+(?:^\h*[^:\v]+\h*:\h*\V*\h*$\v+)*
                                                                                                 ^
	at org.apache.maven.doxia.module.markdown.MarkdownParserTest.setUp(MarkdownParserTest.java:58)

This one I understand. The \h escape sequence in Regex appears to be not supported in Java 7, unfortunately.

So, this needs to be fixed in a separate issue. Sorry about that!

@bertysentry
Copy link
Contributor Author

Before we spend too much time on this, do we want to spend time on compatibility with Java 7?
Java 7 is also what prevents us from using a more recent version of Flexmark, which requires Java 8.

@elharo
Copy link
Contributor

elharo commented Jan 7, 2021

Yes, we absolutely do need to fix this. We have to rollback the offending commit/PR before anything else can move forward.

@bertysentry
Copy link
Contributor Author

Okay, I now understand the problem: the Pattern is compiled as a static constant in MarkdownParser. The Pattern.compile() fails because of the unknown \h escape sequence and then the class is therefore not initialized, and cannot be instantiated as a Plexus component.

I'll submit a new PR to fix this. How do you trigger builds on Jenkins? We could have caught this before.

Thanks!

@elharo
Copy link
Contributor

elharo commented Jan 7, 2021

Unfortunately only committers can trigger Jenkins builds so you have to ask one of us to do it for you. :-(

@bertysentry
Copy link
Contributor Author

It's okay. I can reproduce the problem locally (and thus fix it).

* doxia-module-markdown: Re-incorporated integration tests from **maven-site-plugin**
* doxia-module-markdown: Merged DOXIA-616 integration tests into **general**
@bertysentry bertysentry force-pushed the feature/DOXIA-618-reincorporate-msite-tests branch from 0f6b80f to dce37ef Compare January 7, 2021 19:20
@bertysentry
Copy link
Contributor Author

Okay, so I rebased DOXIA-618 on master so we can see it now builds without any errors (pfew!)
@elharo Could you please trigger a build of this branch/PR?
Thanks!

@elharo elharo changed the title [DOXIA-618] [DOXIA-618] import tests from maven-site-plugin Jan 8, 2021
@elharo
Copy link
Contributor

elharo commented Jan 8, 2021

The branches are a bit confusing but I'm running it through now

@bertysentry
Copy link
Contributor Author

@elharo How is it going?
WRT branches, it's simply 1 commit on top of master. If you pulled this branch into your local git repo, you'll need to pull again as this branch had to be rebased.

@elharo elharo merged commit 8128fd3 into apache:master Jan 11, 2021
@bertysentry bertysentry deleted the feature/DOXIA-618-reincorporate-msite-tests branch January 11, 2021 11:57
@bertysentry
Copy link
Contributor Author

Awesome, thank you!

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.

2 participants