Skip to content

Commit

Permalink
Avoid NPE in config initialization
Browse files Browse the repository at this point in the history
- Configuration had an NPE (which only occured in newer tests)
- As Configuration::isValid threw an error on configuration errors,
  it was migrated to validate.
- provide strong test coverage for Gradle task/plugin
- Drop unused (and untested) main method from AllChecksRunner
  • Loading branch information
ascheman committed Sep 18, 2024
1 parent f60b64a commit 60aa7bc
Show file tree
Hide file tree
Showing 8 changed files with 228 additions and 151 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ public class AllChecksRunner {

/**
* runs all available checks
*
*/

public AllChecksRunner(Configuration configuration) {
Expand All @@ -75,12 +74,12 @@ public AllChecksRunner(Configuration configuration) {
this.checkingResultsDir = configuration.getCheckingResultsDir();
this.junitResultsDir = configuration.getJunitResultsDir();

logger.debug("AllChecksRunner created with " + this.checkers.size() + " checkers for " + filesToCheck.size() + " files");
logger.debug("AllChecksRunner created with {} checkers for {} files",
this.checkers.size(), filesToCheck.size());
}

/**
* Performs all available checks on pageToCheck
*
*/
public PerRunResults performAllChecks() throws IOException {

Expand Down Expand Up @@ -110,9 +109,9 @@ public PerRunResults performAllChecks() throws IOException {
}

/**
* Performs all configured checks on a single HTML file.
* Performs all configured checks on a single HTML file.
* <p>
* Creates a {@link org.aim42.htmlsanitycheck.collect.SinglePageResults} instance to keep checking results.
* Creates a {@link org.aim42.htmlsanitycheck.collect.SinglePageResults} instance to keep checking results.
*/
protected SinglePageResults performChecksForOneFile(File thisFile) throws IOException {

Expand Down Expand Up @@ -179,14 +178,4 @@ private void reportCheckingResultsAsJUnitXml(String resultsDir) {
Reporter reporter = new JUnitXmlReporter(resultsForAllPages, resultsDir);
reporter.reportFindings();
}

/**
* Runs the checks from the command
* line with default settings...
*/
public static void main(String[] args) {
// TODO: read parameter from command line

// Empty method is currently marked "prio-2" issue by CodeNarc... we'll ignore that
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ public class Configuration {
* httpSuccessCodes = [503]
* <p>
* During configuration initialization, the value(s) of httpSuccessCodes will be:
* 1.) set-added to httpSuccessCodes,
* 2.) set-subtracted from the warnings and errors.
* 1.) Set-added to httpSuccessCodes,
* 2.) Set-subtracted from the warnings and errors.
*/
// Make modifiable copies!
final Set<Integer> httpWarningCodes = new HashSet<>(Web.HTTP_WARNING_CODES);
Expand Down Expand Up @@ -75,7 +75,7 @@ public synchronized void setSourceConfiguration(final File sourceDir, final Set<
}

/**
* overwrites httpSuccessCodes configuration
* Overrides httpSuccessCodes configuration
*/
public void overrideHttpSuccessCodes(Set<Integer> additionalSuccessCodes) {
if (null != additionalSuccessCodes) {
Expand All @@ -86,12 +86,12 @@ public void overrideHttpSuccessCodes(Set<Integer> additionalSuccessCodes) {
}
);
} else {
log.warn("Trying to override http success codes with empty set of codes");
log.warn("Trying to override http success codes with an empty set of codes");
}
}

/**
* overwrites httpWarningCodes configuration
* Overrides httpWarningCodes configuration
*/
public void overrideHttpWarningCodes(Set<Integer> additionalWarningCodes) {
if (null != additionalWarningCodes) {
Expand All @@ -102,12 +102,12 @@ public void overrideHttpWarningCodes(Set<Integer> additionalWarningCodes) {
}
);
} else {
log.warn("Trying to override http warning codes with empty set of codes");
log.warn("Trying to override http warning codes with an empty set of codes");
}
}

/**
* overwrites httpErrorCodes configuration
* Overrides httpErrorCodes configuration
*/
public void overrideHttpErrorCodes(Set<Integer> additionalErrorCodes) {
if (null != additionalErrorCodes) {
Expand All @@ -118,17 +118,17 @@ public void overrideHttpErrorCodes(Set<Integer> additionalErrorCodes) {
}
);
} else {
log.warn("Trying to override http error codes with empty set of codes");
log.warn("Trying to override http error codes with an empty set of codes");
}
}

/**
* checks plausibility of configuration:
* We need at least one html file as input, maybe several
* Checks plausibility of configuration:
* We need at least one HTML file as input, maybe several
*/
public Boolean isValid() throws MisconfigurationException {
public void validate() throws MisconfigurationException {

// cannot check if source director is null (= unspecified)
// cannot check if the source directory is null (= unspecified)
if ((sourceDir == null)) {
throw new MisconfigurationException("source directory must not be null");
}
Expand All @@ -154,10 +154,5 @@ public Boolean isValid() throws MisconfigurationException {
if (checksToExecute == null || checksToExecute.isEmpty()) {
throw new MisconfigurationException("checks to execute have to be a non-empty list");
}


// if no exception has been thrown until now,
// the configuration seems to be valid..
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ class ConfigurationSpec extends Specification {

and: "503 is NOT contained in warningCodes"
!myConfig.getHttpWarningCodes().contains(newSuccessCode)

}

def "can overwrite http warning codes"() {
Expand Down Expand Up @@ -88,8 +87,7 @@ class ConfigurationSpec extends Specification {

when: "we create a configuration with this single file"
myConfig.setSourceConfiguration(tempDir, [htmlFile] as Set)

myConfig.isValid()
myConfig.validate()

then:
htmlFile.exists()
Expand All @@ -111,29 +109,26 @@ class ConfigurationSpec extends Specification {
myConfig.setSourceConfiguration(srcDir, srcDocs as Set)

when: "configuration is validated..."
myConfig.isValid()
myConfig.validate()

then: "an exception is thrown"
thrown MisconfigurationException


where:

srcDir | srcDocs
null | null
srcDir | srcDocs
null | null
new File("/_non/exists_/d_ir/") | null
new File("/_non/exists_/d_ir/") | []

// existing but empty directory is absurd too...
File.createTempDir() | []

File.createTempDir() | []
}

// this spec is a syntactic variation of the data (table-)driven test
def "empty configuration makes no sense"() {

when:
myConfig.isValid()
myConfig.validate()

then:
thrown MisconfigurationException
Expand All @@ -151,14 +146,21 @@ class ConfigurationSpec extends Specification {
.build()

when:
myConfig.valid
myConfig.validate()

then:
def e = thrown(MisconfigurationException)
e.message == "checks to execute have to be a non-empty list"

where:
checkersToExecute << [[] as List]
checkersToExecute << [[] as List<Class<? extends Checker>>]
}

}
// The following methods only increase code coverage without providing any test at all
def "cannot overwrite http error codes with null list"() {
expect: "we overwrite the standard Configurations with null"
myConfig.overrideHttpSuccessCodes(null)
myConfig.overrideHttpWarningCodes(null)
myConfig.overrideHttpErrorCodes(null)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import org.gradle.api.tasks.TaskAction
*/
class HtmlSanityCheckTask extends DefaultTask {

//
// we support checking several named files
@InputFiles
FileCollection sourceDocuments
Expand Down Expand Up @@ -79,29 +78,22 @@ class HtmlSanityCheckTask extends DefaultTask {

// private stuff
// **************************************************************************

private Set<File> allFilesToCheck

private Configuration myConfig


/**
* Sets sensible defaults for important attributes.
*
* Ensures that task is _run-always_,
* by setting outputs.upToDateWhen to false.
*/
HtmlSanityCheckTask() {

// Never consider this task up-to-date.
// thx https://github.com/stevesaliman/gradle-cobertura-plugin/commit/d61191f7d5f4e8e89abcd5f3839a210985526648
outputs.upToDateWhen { false }

// give sensible default for output directory, see https://github.com/aim42/htmlSanityCheck/issues/205
checkingResultsDir = new File(project.buildDir, '/reports/htmlSanityCheck/')
junitResultsDir = new File(project.buildDir, '/test-results/htmlSanityCheck/')


checkingResultsDir = new File(project.DEFAULT_BUILD_DIR_NAME, '/reports/htmlSanityCheck/')
junitResultsDir = new File(project.DEFAULT_BUILD_DIR_NAME, '/test-results/htmlSanityCheck/')
}

void setSourceDir(File sourceDir) {
Expand All @@ -112,62 +104,56 @@ class HtmlSanityCheckTask extends DefaultTask {
}
}

/**
* entry point for several html sanity checks
* @author Gernot Starke <[email protected]>
*/
/**
* Entry point for several html sanity checks
* @author Gernot Starke <[email protected]>
*/
@TaskAction
void sanityCheckHtml() {

// tell us about these parameters
logBuildParameter()

// get configuration parameters from gradle
myConfig = setupConfiguration()

// if we have no valid configuration, abort with exception
if (myConfig.isValid()) {

// create output directory for checking results
checkingResultsDir.mkdirs()
assert checkingResultsDir.isDirectory()
assert checkingResultsDir.canWrite()
if (junitResultsDir) {
junitResultsDir.mkdirs()
assert junitResultsDir.isDirectory()
assert junitResultsDir.canWrite()
}
myConfig.validate()

// create output directory for checking results
checkingResultsDir.mkdirs()
assert checkingResultsDir.isDirectory()
assert checkingResultsDir.canWrite()
if (junitResultsDir) {
junitResultsDir.mkdirs()
assert junitResultsDir.isDirectory()
assert junitResultsDir.canWrite()
}

// TODO: unclear: do we need to adjust pathnames if running on Windows(tm)??
// TODO: unclear: do we need to adjust path-names if running on Windows(tm)??

logger.info("buildfile-info", sourceDocuments?.toString())
logger.info("allFilesToCheck" + allFilesToCheck.toString(), "")
logger.info("Source documents: '{}'", sourceDocuments)

// create an AllChecksRunner...
def allChecksRunner = new AllChecksRunner(myConfig)
// create an AllChecksRunner...
def allChecksRunner = new AllChecksRunner(myConfig)

// ... and perform the actual checks
def allChecks = allChecksRunner.performAllChecks()
// ... and perform the actual checks
def allChecks = allChecksRunner.performAllChecks()

// check for findings and fail build if requested
def nrOfFindingsOnAllPages = allChecks.nrOfFindingsOnAllPages()
logger.debug("Found ${nrOfFindingsOnAllPages} error(s) on all checked pages")
// check for findings and fail build if requested
def nrOfFindingsOnAllPages = allChecks.nrOfFindingsOnAllPages()
logger.debug("Found ${nrOfFindingsOnAllPages} error(s) on all checked pages")

if (failOnErrors && nrOfFindingsOnAllPages > 0) {
def failureMsg = """
if (failOnErrors && nrOfFindingsOnAllPages > 0) {
def failureMsg = """
Your build configuration included 'failOnErrors=true', and ${nrOfFindingsOnAllPages} error(s) were found on all checked pages.
See ${checkingResultsDir} for a detailed report."""
throw new GradleException(failureMsg)
}
} else {
logger.warn("""Fatal configuration errors preventing checks:\n
${myConfig.toString()}""")
throw new GradleException(failureMsg)
}
}

/**
* setup a @Configuration instance containing all given configuration parameters
* from the gradle buildfile.
* from the gradle build-file.
*
* This method has to be updated in case of new configuration parameters!!
*
Expand All @@ -177,7 +163,7 @@ See ${checkingResultsDir} for a detailed report."""
protected Configuration setupConfiguration() {

Configuration result = Configuration.builder()
.sourceDocuments(sourceDocuments.files)
.sourceDocuments(sourceDocuments?.files)
.sourceDir(sourceDir)
.checkingResultsDir(checkingResultsDir)
.junitResultsDir(junitResultsDir)
Expand Down Expand Up @@ -210,16 +196,13 @@ See ${checkingResultsDir} for a detailed report."""

private void logBuildParameter() {
logger.info "=" * 70
logger.info "Parameters given to sanityCheck plugin from gradle buildfile..."
logger.info "Parameters given to sanityCheck plugin from gradle build-file..."
logger.info "Files to check : $sourceDocuments"
logger.info "Source directory: $sourceDir"
logger.info "Results dir : $checkingResultsDir"
logger.info "JUnit dir : $junitResultsDir"
logger.info "Fail on errors : $failOnErrors"

}


}

/*========================================================================
Expand Down
Loading

0 comments on commit 60aa7bc

Please sign in to comment.