[Test Cleanup] First attempt #828

kalaspuffar · 2018-01-08T21:17:48Z

Moved the API constructor tests to a new package and changed it to use a minimal epub for testing.

The earlier implementation had tests that generated one warning and one error in the expected result. I don't really know what that would do to validate the test more. It's an isolated test with little dependencies. But I thought it was nicer to use a minimal implementation instead.

It required a binary version of the API calls though if I understood the implementation so I had to package one. Please inform me if I'm wrong at this assumption.

I also invented the new package org.w3.epubcheck.

I'm not sure if this is a good idea but my reasoning was that we want a new place to put all the test. We want to reuse the names of the different packages for the tests but we still want to know what is new and what is old. And I don't know the history but does Adobe have anything to do with the project today?

I thought that a more generic package as org.w3 could be good. Maybe the main implementation could be moved to this package in the future.

Thoughts?

Best regards
Daniel

tofi86

Hi Daniel,

thanks for your work. A new package org.w3 sounds good to me. And no, Adobe has nothing to do with the project anymore.

However, I'm not sure whether we should mix all this, together with package org.idpf for css stuff we come up with a third namespace.
Probably we should just move all the code? But then we couldn' differ between old and new... I'm not sure...

Regarding the refactoring: Probably it was intended that the Api tests will also throw errors in certain cases to just test the output of errors from the Api?

Do we need a test which tests a non-valid epub here?

… a minimal epub for testing.

kalaspuffar · 2018-11-30T13:33:41Z

Hi @tofi86

I've continued to work on this today. Looking at the rest of the tests in API and I realize that if we want to restructure and verify each test the API test will be relocated to multiple new directories.

We and I also found that we can't use the suggested name package for package tests (a reserved word). So I use the name pkg instead. And moreover, this work, as extensive as it is, might be better to be done in one pull request or at least by one person. The requirement to find duplication and also see if we have missed some test case can be hard to overview if you do it in separate branches and with a lot of people involved.

I will continue to refactor the API tests and add them into the correct structure under org.idpf in order not to introduce another package scope. But that means that the util directory will not be refactored at the moment.

Another reason why it's a bad thing to spread this work out on multiple concurrent branches is the problem with merge conflicts.

This message might be a bit unstructured but I'm in the middle of the work and just wanted to raise some issues without giving too much thought to them.

Best regards
Daniel

tofi86 · 2018-12-03T09:39:32Z

Hey Daniel,

I'm afraid I don't have that much time at the moment do help out on this issue. But thanks a lot for your work on this refactoring task.

Since Daisy took over the maintenance of the project recently, I think @rdeltour will be happy to help out here and answer your questions.

Best,
Tobias

kalaspuffar · 2018-12-03T15:02:57Z

Hi @rdeltour

I will be working full-time this week on test rewrite if nothing unexpected happens.

My plan for this pull request is to rework API and Command line. Also did some work on another set of API tests that was wrongly labelled in my eyes.

This rework builds a lot of structure so each batch will build on the last.

So if you can give me some pointers during the process it would be appreciated so I don't have to rework a lot later.

Best regards
Daniel

rdeltour · 2018-12-03T19:05:24Z

hi @kalaspuffar

My plan for this pull request is to rework API and Command line.

as these are potentially user-facing changes, maybe we should schedule a quick chat to talk about it first? Feel free to contact me over email at rdeltour(at)gmail(dot)com.

We're in the process of implementing support for EPUB 3.2; so let's try to minimize the refactoring conflicts 😊

rdeltour

I've rebased the tests to the ongoing master, resolved the merge conflict and re-introduced the changes made to these tests since the original PR (see branch pr/828/test-cleanup).

The only comment I have left is about the removal of the IT test, which I don't think was properly replace…

rdeltour · 2019-07-12T14:22:18Z

src/test/java/com/adobe/epubcheck/cli/CheckerIT.java

-public class CheckerIT
-{
-
-  private static final String[] cmd = new String[] { "java", "-jar", "target/epubcheck.jar" };


This test was an integration test; its role was to test that EPUBCheck launched as an external process matched the expected output.

This doesn't seem to have been moved anywhere, and it's not covered by the new CommandLineTest. Am I missing something?

Hi @rdeltour

Yes, I believe that this test is not moved. It's been a while since I did this pull request so I might remember the reason wrongly.

But my best guess is that we have multiple tests checking that different command line variants works and returns the expected output so having one extra without any parameters would just fail when all the others fail. So it would not give us any extra information IMHO.

If this is an incorrect assumption then we just need to add another command line test using the function that doesn't have any parameters.

Best regards
Daniel

Putting this as another test in the CommandLineTest class will not work: this test is specifically an integration test. It launches Java as an external process, so that we can expect the result output including bootstrap messages etc.

For instance, this was needed to test a regression with some messages produced by Saxon only at the JVM launch time. If the test is run on an existing JVM, where the EPUBCheck classes were already initialized, then these messages would no longer happen.

For reference, this test was included in #910.

If you see nothing wrong, I'll re-introduce this test in my integration branch pr/828/test-cleanup, and will then merge the PR…

@kalaspuffar

Based on @kalaspuffar’s work on #828 - use the locale object as the `Messages` lookup key to work around test failing when the messages created with the US default locale were retrieved when looking up the English locale - add test dependency to `com.googlecode.json-simple:json-simple` for easy JSON parsing - minor cleanup of `AbstractEpubCheckTest` - cleanup `api_Test` to `ApiConstructosTest` - consolidate CLI tests from `CLITest` and `command_line_Test` to a new `CommandLineTest` test class - rename class `com.adobe.epubcheck.test.common` to `CommonTestRunner` - add dreprecation annotation to the following test classes, pending refactoring: - CommonTestRunner - JsonCompare - Epub20CheckTest - Epub20CheckExpandedTest - Epub30CheckTest - Epub30CheckExpandedTest - LocalizationTest - NavCheckerTest - OCFCheckerTest - OCFFilenameCheckerTest - OCFMockPackage - MetadataSetTest - OPFCheckerTest - OPFCheckerTestChecks - OPSCheckerTest - OverlayCheckerTest - StressTest - MessageDictionary_LocalizationTest - OutputDifferenceListener - OverriddenMessageDictionaryTest - TestRunListener - UtilMessage_LocalizationTest - css_Test - debug - dtBook_Test - encryption_Test - message_coverage - opf_Test - package_Test - script_Test - single_file_Test - toc_Test - xhtml_Test - ExtraReportTest - OPFPeekerTest - PathUtilTest - ValidationReport - PrefixParsingTest - PropertyTest - VocabTest

rdeltour · 2019-07-19T09:32:57Z

I integrated and adapted your PR in a new PR #1061 :

rebased to a work-in-progress test/cleanup branch (itself based off the current master head)
re-introduced the integration test
reverted the changes Epub20CheckExpandedTest, until we do the forthcoming systematic refactoring of these tests
- other minor tweaks

Closing this PR now, as it's overridden by #1061.

Thanks again @kalaspuffar for the contribution!

@kalaspuffar

#1061) Based on @kalaspuffar’s work on #828 - use the locale object as the `Messages` lookup key to work around test failing when the messages created with the US default locale were retrieved when looking up the English locale - add test dependency to `com.googlecode.json-simple:json-simple` for easy JSON parsing - minor cleanup of `AbstractEpubCheckTest` - cleanup `api_Test` to `ApiConstructosTest` - consolidate CLI tests from `CLITest` and `command_line_Test` to a new `CommandLineTest` test class - rename class `com.adobe.epubcheck.test.common` to `CommonTestRunner` - add dreprecation annotation to the following test classes, pending refactoring: - CommonTestRunner - JsonCompare - Epub20CheckTest - Epub20CheckExpandedTest - Epub30CheckTest - Epub30CheckExpandedTest - LocalizationTest - NavCheckerTest - OCFCheckerTest - OCFFilenameCheckerTest - OCFMockPackage - MetadataSetTest - OPFCheckerTest - OPFCheckerTestChecks - OPSCheckerTest - OverlayCheckerTest - StressTest - MessageDictionary_LocalizationTest - OutputDifferenceListener - OverriddenMessageDictionaryTest - TestRunListener - UtilMessage_LocalizationTest - css_Test - debug - dtBook_Test - encryption_Test - message_coverage - opf_Test - package_Test - script_Test - single_file_Test - toc_Test - xhtml_Test - ExtraReportTest - OPFPeekerTest - PathUtilTest - ValidationReport - PrefixParsingTest - PropertyTest - VocabTest

@kalaspuffar

#1061) Based on @kalaspuffar’s work on #828 - use the locale object as the `Messages` lookup key to work around test failing when the messages created with the US default locale were retrieved when looking up the English locale - add test dependency to `com.googlecode.json-simple:json-simple` for easy JSON parsing - minor cleanup of `AbstractEpubCheckTest` - cleanup `api_Test` to `ApiConstructosTest` - consolidate CLI tests from `CLITest` and `command_line_Test` to a new `CommandLineTest` test class - rename class `com.adobe.epubcheck.test.common` to `CommonTestRunner` - add dreprecation annotation to the following test classes, pending refactoring: - CommonTestRunner - JsonCompare - Epub20CheckTest - Epub20CheckExpandedTest - Epub30CheckTest - Epub30CheckExpandedTest - LocalizationTest - NavCheckerTest - OCFCheckerTest - OCFFilenameCheckerTest - OCFMockPackage - MetadataSetTest - OPFCheckerTest - OPFCheckerTestChecks - OPSCheckerTest - OverlayCheckerTest - StressTest - MessageDictionary_LocalizationTest - OutputDifferenceListener - OverriddenMessageDictionaryTest - TestRunListener - UtilMessage_LocalizationTest - css_Test - debug - dtBook_Test - encryption_Test - message_coverage - opf_Test - package_Test - script_Test - single_file_Test - toc_Test - xhtml_Test - ExtraReportTest - OPFPeekerTest - PathUtilTest - ValidationReport - PrefixParsingTest - PropertyTest - VocabTest

rkwright requested review from tofi86 and rkwright February 27, 2018 15:12

tofi86 reviewed Feb 27, 2018

View reviewed changes

tofi86 added the refactoring label Feb 27, 2018

tofi86 assigned kalaspuffar Feb 27, 2018

rdeltour added type: improvement The issue suggests an improvement of an existing feature and removed type: improvement The issue suggests an improvement of an existing feature refactoring labels Nov 12, 2018

kalaspuffar force-pushed the test_cleanup_firsttest branch from 3877a0d to 43eb39b Compare November 30, 2018 07:41

kalaspuffar added 2 commits November 30, 2018 08:45

Moved the api constructor test to a new package and changed it to use…

d9bb13b

… a minimal epub for testing.

Added documentation.

43eb39b

kalaspuffar changed the title ~~[Test Cleanup] First attempt~~ [WIP] [Test Cleanup] First attempt Nov 30, 2018

kalaspuffar added 5 commits November 30, 2018 15:39

Reworking more tests.

9a548e8

Remove incorrect char.

90adea7

Added comments.

d106428

Coding standard.

140c300

Some more moving around.

eefefd0

kalaspuffar added 2 commits December 3, 2018 13:29

Correct java functions

ae3e916

End day with passing tests

ac6ef3e

Added all severity tests.

fc6da3a

rdeltour requested review from rdeltour and removed request for rkwright December 4, 2018 09:49

kalaspuffar added 3 commits December 4, 2018 11:19

Added fail on warnings test.

abb245f

Move some tests back and add deprecation.

4c1c348

Added output validation tests.

9db63f4

kalaspuffar added 3 commits December 7, 2018 15:00

Moved more CLI functions.

96b3f9c

Converted tests for help and version.

2e3b236

Translated a few more tests.

e554cc9

kalaspuffar changed the title ~~[WIP] [Test Cleanup] First attempt~~ [Test Cleanup] First attempt Dec 10, 2018

Final changes for first batch.

d54b509

rdeltour requested changes Jul 12, 2019

View reviewed changes

rdeltour mentioned this pull request Jul 19, 2019

test: clean API constructor test and CLI tests + deprecate other tests #1061

Merged

rdeltour closed this Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Test Cleanup] First attempt #828

[Test Cleanup] First attempt #828

kalaspuffar commented Jan 8, 2018

tofi86 left a comment

kalaspuffar commented Nov 30, 2018

tofi86 commented Dec 3, 2018

kalaspuffar commented Dec 3, 2018

rdeltour commented Dec 3, 2018

rdeltour left a comment

rdeltour Jul 12, 2019

kalaspuffar Jul 13, 2019

rdeltour Jul 15, 2019

rdeltour Jul 15, 2019

rdeltour Jul 15, 2019

rdeltour commented Jul 19, 2019

[Test Cleanup] First attempt #828

[Test Cleanup] First attempt #828

Conversation

kalaspuffar commented Jan 8, 2018

tofi86 left a comment

Choose a reason for hiding this comment

kalaspuffar commented Nov 30, 2018

tofi86 commented Dec 3, 2018

kalaspuffar commented Dec 3, 2018

rdeltour commented Dec 3, 2018

rdeltour left a comment

Choose a reason for hiding this comment

rdeltour Jul 12, 2019

Choose a reason for hiding this comment

kalaspuffar Jul 13, 2019

Choose a reason for hiding this comment

rdeltour Jul 15, 2019

Choose a reason for hiding this comment

rdeltour Jul 15, 2019

Choose a reason for hiding this comment

rdeltour Jul 15, 2019

Choose a reason for hiding this comment

rdeltour commented Jul 19, 2019