Reviewers: MarkM,
Description:
* getCurrentSeverity would report the severity as if 'permit' is false;
fixed by checking permit inside getCurrentSeverity.
* 'doNotRepair' would cause a problem to never be removed from
yetToRepair; fixed by having runTests check doNotRepair.
Supporting changes:
* Added tests for acceptableProblems in test-repair-framework.
* Verify that acceptableProblems is not modified after it is used.
Please review this at https://codereview.appspot.com/61930043/
Affected files (+68, -5 lines):
M src/com/google/caja/ses/repair-framework.js
M tests/com/google/caja/ses/test-repair-framework.js
Index: src/com/google/caja/ses/repair-framework.js
===================================================================
--- src/com/google/caja/ses/repair-framework.js (revision 5663)
+++ src/com/google/caja/ses/repair-framework.js (working copy)
@@ -245,6 +245,12 @@
var acceptableProblems = {};
/**
+ * Whether acceptableProblems has been used and therefore should not be
+ * modified.
+ */
+ var acceptableProblemsLocked = false;
+
+ /**
* As we start to repair, this will track the worst *post-repair*
severity
* seen so far.
*
@@ -308,6 +314,7 @@
var defaultDisposition = { permit: false, doNotRepair: false };
function disposition(problem) {
+ acceptableProblemsLocked = true;
return Object.prototype.hasOwnProperty.call(acceptableProblems,
problem.id) ? acceptableProblems[problem.id] :
defaultDisposition;
}
@@ -335,7 +342,8 @@
repairsPerformed.lastIndexOf(problem.repair) !== -1;
// Update yetToRepair and plannedSeverity
- if (repairPerformed || !problem.repair) { // repair
attempted/absent
+ if (repairPerformed || !problem.repair ||
+ disposition(problem).doNotRepair) { // repair attempted/absent
if (report.postSeverity.level > severities.SAFE.level
&& disposition(problem).permit) {
@@ -452,7 +460,9 @@
};
this.setAcceptableProblems = function(value) {
- // TODO(kpreid): Check some condition? Do only once?
+ if (acceptableProblemsLocked) {
+ throw new Error('Too late to setAcceptableProblems.');
+ }
acceptableProblems = value;
};
@@ -469,9 +479,8 @@
this.getCurrentSeverity = function() {
var severity = plannedSeverity;
yetToRepair.forEach(function(problem) {
- // TODO(kpreid): Fix interaction of this with acceptableProblems
config
-
- if (problem.preSeverity.level > severity.level) {
+ if (problem.preSeverity.level > severity.level &&
+ !disposition(problem).permit) {
severity = problem.preSeverity;
}
});
@@ -504,6 +513,11 @@
// TODO(kpreid): Replace uses of this with higher level ops
this.updateMaxSeverity = function updateMaxSeverity(severity) {
if (severity.level > plannedSeverity.level) {
+ // This is a useful breakpoint for answering the question "why is
the
+ // severity as high as it is".
+ // if (severity.level > maxAcceptableSeverity.level) {
+ // console.info('Increasing planned severity.');
+ // }
plannedSeverity = severity;
}
};
Index: tests/com/google/caja/ses/test-repair-framework.js
===================================================================
--- tests/com/google/caja/ses/test-repair-framework.js (revision 5663)
+++ tests/com/google/caja/ses/test-repair-framework.js (working copy)
@@ -141,6 +141,55 @@
jsunitPass();
});
+ jsunitRegister('testAcceptableProblems', function() {
+ var repairer = new ses._Repairer();
+
+ repairer.setAcceptableProblems({
+ 'DNR': { doNotRepair: true },
+ 'PERMIT': { permit: true },
+ 'PERMIT_DNR': { permit: true, doNotRepair: true },
+ });
+ var repaired_pd = false;
+ var repaired_d = false;
+ repairer.registerProblem({
+ id: 'PERMIT',
+ test: function() { return true; },
+ repair: undefined,
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
+ canRepair: false,
+ });
+ repairer.registerProblem({
+ id: 'DNR',
+ test: function() { return !repaired_d; },
+ repair: function() { repaired_d = true; },
+ preSeverity: severities.SAFE_SPEC_VIOLATION,
+ canRepair: true,
+ });
+ repairer.registerProblem({
+ id: 'PERMIT_DNR',
+ test: function() { return !repaired_pd; },
+ repair: function() { repaired_pd = true; },
+ preSeverity: severities.UNSAFE_SPEC_VIOLATION,
+ canRepair: true,
+ });
+ repairer.runTests('test without repair');
+ assertEquals('cur sev 1', severities.SAFE_SPEC_VIOLATION,
+ repairer.getCurrentSeverity());
+ assertEquals('plan sev 1', severities.SAFE_SPEC_VIOLATION,
+ repairer.getPlannedSeverity());
+ repairer.testAndRepair();
+ assertFalse('not repaired DNR', repaired_d);
+ assertFalse('not repaired permit&DNR', repaired_pd);
+ // We expect SAFE_SPEC_VIOLATION because problem 'DNR' is doNotRepair
but
+ // it is not permitted, so its severity should show up.
+ assertEquals('cur sev 2', severities.SAFE_SPEC_VIOLATION,
+ repairer.getCurrentSeverity());
+ assertEquals('plan sev 2', severities.SAFE_SPEC_VIOLATION,
+ repairer.getPlannedSeverity());
+
+ jsunitPass();
+ });
+
jsunitRegister('testRepairOutcomes', function() {
var repairer = new ses._Repairer();
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
