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

introduce integration tests with minimum needed changes #301

Merged
merged 9 commits into from
Dec 31, 2018

Conversation

yasserzamani
Copy link
Member

  • upgrades jwebunit to latest available version
  • downgrades to jquery 1.8.2 and bootstrap 2.0.3 for jwebunit compatibility

- upgrades jwebunit to latest available version
- downgrades to jquery 1.8.2 and bootstrap 2.0.3 for jwebunit compatibility
@coveralls
Copy link

coveralls commented Dec 24, 2018

Coverage Status

Coverage increased (+1.08%) to 48.003% when pulling 71a1d40 on yasserzamani:integration_tests into 45e2dd4 on apache:master.

@aleksandr-m
Copy link
Contributor

Why downgrade jQuery and Bootstrap?

@yasserzamani
Copy link
Member Author

I didn't probe about the reason but jwebunit release has been abolished after Oct 2015 :( it's latest release isn't able to execute jQuery greater than 1.8.2. And the last bootstrap which is able to work with jQuery 1.8.2 is bootstrap 2.0.3.

@aleksandr-m
Copy link
Contributor

How about using HtmlUnit directly or some other testing framework. jQuery 1.8.2 probably is full of security vulnerabilities. Using ancient dependencies because of web testing, I would say: no go.

@yasserzamani
Copy link
Member Author

I thought normally no one serves showcase publicly on a production server, additionally I thought java script as a client side execution couldn't be so dangerous menace, so I thought it's OK to postpone switching to other testing framework to another PR as it could be a burden. I would focus on integration tests and their coverage report on this PR with minimum changes to not confuse reviewers. Anyway, you're right and we must keep up to date 👍

@apache/struts-committers , gathering integration tests coverage using current available tools had different levels of hardship. According to my numerous probes in internet my PR finally should work but I'm baffled why it reports 0% coverage on classes that jetty covers 😕 please advice if any working example or idea. The first phase is the showcase classes itself. The second phase is e.g. when we have an integration test in showcase which uses e.g. a class from DWR plugin module recording it. I think it needed to add dependencies not as jar but as classes but this idea is thoroughly absurd according to maven architecture and philosophy.

@lukaszlenart
Copy link
Member

As I see you did it :)

@yasserzamani
Copy link
Member Author

Yes it seems I'm now more near :) -- still no coverage for showcase classes but when I locally comment <jvmArgs>${argLine}</jvmArgs> jacoco generates a small jacoco-it.exec file which doesn't contain showcase classes listed - currently it contains them but with 0 as coverage. These mean that jvmArgs makes sense :)

I guess there is only a morsel of tricky things is remained to achieve this goal. But as you mentioned, I would conclude this PR at here as good enough - if @aleksandr-m approves about jquery, I think it's now OK and it excels forward :)

@aleksandr-m
Copy link
Contributor

Do integration tests work now?

currently it contains them but with 0 as coverage

Does this means no? What benefit this PR brings then?

Why not to extend this PR with more up to date web testing framework?

We should really upgrade jQuery and Bootstrap to latest versions.

@yasserzamani
Copy link
Member Author

Do integration tests work now?

Yes they do. Please see here and prior lines.

Does this means no? What benefit this PR brings then?

No. Those integration tests are running and passing. I meant recording their coverage doesn't work comprehensively. However, integration tests coverage recording isn't an essential and popular item.

Why not to extend this PR with more up to date web testing framework?
We should really upgrade jQuery and Bootstrap to latest versions.

As I confirmed, yes we must. However, I personally liked to merge this one prior to coming soon releases to be more confident about them then upgrade web testing framework, jQuery and Bootstrap in coming soon next PR as I don't have an estimation how much time and effort is needed for them. wdyt?

@aleksandr-m
Copy link
Contributor

Let's wait a bit. I'll take a look.

@yasserzamani
Copy link
Member Author

Sure. Thanks a lot! I was devised replace jwebunit-htmlunit-plugin with jwebunit-selenium-plugin as first try. No need to rewrite tests and it might work because it has old but latest selenium dependency.

@aleksandr-m
Copy link
Contributor

I've managed to convert most of the tests using HtmlUnit. Just a few tricky tests left. Stay tuned... :)

@yasserzamani
Copy link
Member Author

Wow! you did it, those warning about css also vanished now 🥇 👏 thanks a lot! I'll squash and merge them :)

@yasserzamani yasserzamani merged commit fa90e01 into apache:master Dec 31, 2018
@yasserzamani
Copy link
Member Author

I'm sorry @aleksandr-m , I squashed to avoid spamming git log and history but GitHub has merged your works with my name :( excuse me as I didn't know this.

@aleksandr-m
Copy link
Contributor

@yasserzamani Squash tends to do that. :) Don't sweat it. Though personally I would merge this particular PR instead of squashing it. Besides few typo fixing commits it has clear logically separated commits.

@yasserzamani yasserzamani deleted the integration_tests branch January 23, 2019 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants