Skip to content

Commit 5457f24

Browse files
patrick-skopatrick-sko
committed
Addressed PR Comments
1 parent f949e8b commit 5457f24

File tree

2 files changed

+134
-82
lines changed

2 files changed

+134
-82
lines changed

‎src/com/google/javascript/jscomp/ProductionCoverageInstrumentationCallback.java

Lines changed: 84 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,17 @@ final class ProductionCoverageInstrumentationCallback
6464
private final ParameterMapping parameterMapping;
6565
boolean visitedInstrumentCodeFile = false;
6666

67-
private final String FUNCTION_TYPE = "Type.FUNCTION";
68-
private final String BRANCH_TYPE = "Type.BRANCH";
69-
private final String BRANCH_DEFAULT_TYPE = "Type.BRANCH_DEFAULT";
67+
private enum Type {
68+
FUNCTION("Type.FUNCTION"),
69+
BRANCH("Type.BRANCH"),
70+
BRANCH_DEFAULT("Type.BRANCH_DEFAULT");
71+
72+
private String value;
73+
74+
Type(String s){
75+
this.value = s;
76+
}
77+
}
7078

7179
/**
7280
* Stores a stack of function names that encapsulates the children nodes being instrumented. The
@@ -117,64 +125,74 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
117125

118126
String functionName = functionNameStack.peek();
119127

120-
if (node.isFunction()) {
121-
// If the function node has been visited by visit() then we can be assured that all its
122-
// children nodes have been visited and properly instrumented.
123-
functionNameStack.pop();
124-
instrumentBlockNode(node.getLastChild(), fileName, functionName, FUNCTION_TYPE);
125-
} else if (node.isIf()) {
126-
if (node.getChildCount() == 2) {
127-
addDefaultBlock(node);
128-
}
129-
Node ifTrueNode = node.getSecondChild();
130-
Node ifFalseNode = node.getLastChild();
131-
instrumentBlockNode(ifTrueNode, sourceFileName, functionName, BRANCH_TYPE);
132-
instrumentBlockNode(ifFalseNode, sourceFileName, functionName, BRANCH_DEFAULT_TYPE);
133-
} else if (node.isSwitch()) {
134-
boolean hasDefaultCase = false;
135-
for (Node c = node.getSecondChild(); c != null; c = c.getNext()) {
136-
if (c.isDefaultCase()) {
137-
instrumentBlockNode(c.getLastChild(), sourceFileName, functionName, BRANCH_DEFAULT_TYPE);
138-
hasDefaultCase = true;
139-
} else {
140-
instrumentBlockNode(c.getLastChild(), sourceFileName, functionName, BRANCH_TYPE);
128+
switch (node.getToken()) {
129+
case FUNCTION:
130+
// If the function node has been visited by visit() then we can be assured that all its
131+
// children nodes have been visited and properly instrumented.
132+
functionNameStack.pop();
133+
instrumentBlockNode(node.getLastChild(), fileName, functionName, Type.FUNCTION.value);
134+
break;
135+
case IF:
136+
Node ifTrueNode = node.getSecondChild();
137+
instrumentBlockNode(ifTrueNode, sourceFileName, functionName, Type.BRANCH.value);
138+
if (node.getChildCount() == 2) {
139+
addElseBlock(node);
140+
}
141+
Node ifFalseNode = node.getLastChild();
142+
if (NodeUtil.isEmptyBlock(ifFalseNode)
143+
|| (ifFalseNode.getFirstChild() != null && !ifFalseNode.getFirstChild().isIf())) {
144+
instrumentBlockNode(ifFalseNode, sourceFileName, functionName, Type.BRANCH_DEFAULT.value);
145+
}
146+
break;
147+
case SWITCH:
148+
boolean hasDefaultCase = false;
149+
for (Node c = node.getSecondChild(); c != null; c = c.getNext()) {
150+
if (c.isDefaultCase()) {
151+
instrumentBlockNode(
152+
c.getLastChild(), sourceFileName, functionName, Type.BRANCH_DEFAULT.value);
153+
hasDefaultCase = true;
154+
} else {
155+
instrumentBlockNode(c.getLastChild(), sourceFileName, functionName, Type.BRANCH.value);
156+
}
157+
}
158+
if (!hasDefaultCase) {
159+
Node defaultBlock = IR.block();
160+
defaultBlock.useSourceInfoIfMissingFromForTree(node);
161+
Node defaultCase = IR.defaultCase(defaultBlock).useSourceInfoIfMissingFromForTree(node);
162+
node.addChildToBack(defaultCase);
163+
instrumentBlockNode(
164+
defaultBlock, sourceFileName, functionName, Type.BRANCH_DEFAULT.value);
165+
}
166+
break;
167+
case HOOK:
168+
Node ifTernaryIsTrueExpression = node.getSecondChild();
169+
Node ifTernaryIsFalseExpression = node.getLastChild();
170+
171+
addInstrumentationNodeWithComma(
172+
ifTernaryIsTrueExpression, sourceFileName, functionName, Type.BRANCH.value);
173+
addInstrumentationNodeWithComma(
174+
ifTernaryIsFalseExpression, sourceFileName, functionName, Type.BRANCH.value);
175+
176+
compiler.reportChangeToEnclosingScope(node);
177+
break;
178+
case OR:
179+
case AND:
180+
case COALESCE:
181+
// Only instrument the second child of the binary operation because the first child will
182+
// always execute, or the first child is part of a chain of binary operations and would have
183+
// already been instrumented.
184+
Node secondExpression = node.getLastChild();
185+
addInstrumentationNodeWithComma(
186+
secondExpression, sourceFileName, functionName, Type.BRANCH.value);
187+
188+
compiler.reportChangeToEnclosingScope(node);
189+
break;
190+
default:
191+
if (NodeUtil.isLoopStructure(node)) {
192+
Node blockNode = NodeUtil.getLoopCodeBlock(node);
193+
checkNotNull(blockNode);
194+
instrumentBlockNode(blockNode, sourceFileName, functionName, Type.BRANCH.value);
141195
}
142-
}
143-
if (!hasDefaultCase){
144-
Node defaultBlock = IR.block();
145-
defaultBlock.useSourceInfoIfMissingFromForTree(node);
146-
Node defaultCase = IR.defaultCase(defaultBlock).useSourceInfoIfMissingFromForTree(node);
147-
node.addChildToBack(defaultCase);
148-
instrumentBlockNode(defaultBlock, sourceFileName, functionName, BRANCH_DEFAULT_TYPE);
149-
}
150-
} else if (NodeUtil.isLoopStructure(node)) {
151-
Node blockNode = NodeUtil.getLoopCodeBlock(node);
152-
checkNotNull(blockNode);
153-
instrumentBlockNode(blockNode, sourceFileName, functionName, BRANCH_TYPE);
154-
155-
Node newNode =
156-
newInstrumentationNode(blockNode, sourceFileName, functionName, BRANCH_DEFAULT_TYPE);
157-
blockNode.getGrandparent().addChildAfter(newNode, blockNode.getParent());
158-
compiler.reportChangeToEnclosingScope(blockNode.getParent());
159-
} else if (node.isHook()) {
160-
Node ifTernaryIsTrueExpression = node.getSecondChild();
161-
Node ifTernaryIsFalseExpression = node.getLastChild();
162-
163-
addInstrumentationNodeWithComma(
164-
ifTernaryIsTrueExpression, sourceFileName, functionName, BRANCH_TYPE);
165-
addInstrumentationNodeWithComma(
166-
ifTernaryIsFalseExpression, sourceFileName, functionName, BRANCH_TYPE);
167-
168-
compiler.reportChangeToEnclosingScope(node);
169-
} else if (node.isOr() || node.isAnd() || node.isNullishCoalesce()) {
170-
// Only instrument the second child of the binary operation because the first child will
171-
// always execute, or the first child is part of a chain of binary operations and would have
172-
// already been instrumented.
173-
Node secondExpression = node.getLastChild();
174-
addInstrumentationNodeWithComma(
175-
secondExpression, sourceFileName, functionName, BRANCH_TYPE);
176-
177-
compiler.reportChangeToEnclosingScope(node);
178196
}
179197
}
180198

@@ -185,16 +203,16 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
185203
private void addInstrumentationNodeWithComma(
186204
Node originalNode, String fileName, String functionName, String type) {
187205
Node parentNode = originalNode.getParent();
188-
parentNode.removeChild(originalNode);
206+
Node cloneOfOriginal = originalNode.cloneTree();
189207
Node newInstrumentationNode =
190-
newInstrumentationNode(originalNode, fileName, functionName, type);
208+
newInstrumentationNode(cloneOfOriginal, fileName, functionName, type);
191209

192210
// newInstrumentationNode returns an EXPR_RESULT which cannot be a child of a COMMA node.
193211
// Instead we use the child of of the newInstrumentatioNode which is a CALL node.
194212
Node childOfInstrumentationNode = newInstrumentationNode.getFirstChild().detach();
195213
Node infusedExp =
196-
StatementFusion.fuseExpressionIntoExpression(childOfInstrumentationNode, originalNode);
197-
parentNode.addChildToBack(infusedExp);
214+
StatementFusion.fuseExpressionIntoExpression(childOfInstrumentationNode, cloneOfOriginal);
215+
parentNode.replaceChild(originalNode, infusedExp);
198216
}
199217

200218
/**
@@ -245,8 +263,8 @@ private Node newInstrumentationNode(Node node, String fileName, String fnName, S
245263
return exprNode.useSourceInfoIfMissingFromForTree(node);
246264
}
247265

248-
/** Add a default block for If statements */
249-
private Node addDefaultBlock(Node node) {
266+
/** Add an else block for If statements if one is not already present. */
267+
private Node addElseBlock(Node node) {
250268
Node defaultBlock = IR.block();
251269
node.addChildToBack(defaultBlock);
252270
return defaultBlock.useSourceInfoIfMissingFromForTree(node);

‎test/com/google/javascript/jscomp/integration/ProductionCoverageInstrumentationPassIntegrationTest.java

Lines changed: 50 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static com.google.common.truth.Truth.assertWithMessage;
2121

2222
import com.google.common.annotations.GwtIncompatible;
23+
import com.google.common.base.Strings;
2324
import com.google.common.collect.ImmutableMap;
2425
import com.google.javascript.jscomp.BlackHoleErrorManager;
2526
import com.google.javascript.jscomp.Compiler;
@@ -78,7 +79,7 @@ public void testFunctionInstrumentation() {
7879
String expected =
7980
lines(
8081
"function foo() { ",
81-
getInstrumentCodeLine("C", 1, 0), ";",
82+
getInstrumentCodeLine("C", 1, 0), ";",
8283
" console.log('Hello'); ",
8384
"}");
8485

@@ -113,7 +114,7 @@ public void testNoTranspilationFunctionInstrumentation() {
113114
String expected =
114115
lines(
115116
"function foo() { ",
116-
getInstrumentCodeLine("C", 1, 0), ";",
117+
getInstrumentCodeLine("C", 1, 0), ";",
117118
" console.log('Hello'); ",
118119
"}");
119120

@@ -136,10 +137,40 @@ public void testIfInstrumentation() {
136137
String expected =
137138
lines(
138139
"if (tempBool) { ",
139-
getInstrumentCodeLine("C", 1, 0), ";",
140+
getInstrumentCodeLine("C", 1, 0), ";",
140141
" console.log('Hello'); ",
141142
"} else {",
142-
getInstrumentCodeLine("E", 1, 0), ";",
143+
getInstrumentCodeLine("E", 1, 0), ";",
144+
"}");
145+
146+
String[] expectedArr = {instrumentCodeExpected, expected};
147+
test(options, sourceArr, expectedArr);
148+
}
149+
150+
@Test
151+
public void testIfElseInstrumentation() {
152+
CompilerOptions options = createCompilerOptions();
153+
154+
String source =
155+
lines(
156+
"if (tempBool) {",
157+
" console.log('Hello');",
158+
"} else if(anotherTempBool) {",
159+
" console.log('hi');",
160+
"}");
161+
162+
String[] sourceArr = {instrumentCodeSource, source};
163+
164+
String expected =
165+
lines(
166+
"if (tempBool) {",
167+
getInstrumentCodeLine("C", 1, 0), ";",
168+
" console.log('Hello');",
169+
"} else if(anotherTempBool) {",
170+
getInstrumentCodeLine("C", 3, 7), ";",
171+
" console.log('hi');",
172+
"} else {",
173+
getInstrumentCodeLine("E", 3, 7), ";",
143174
"}");
144175

145176
String[] expectedArr = {instrumentCodeExpected, expected};
@@ -236,10 +267,9 @@ public void testForLoopInstrumentation() {
236267
String expected =
237268
lines(
238269
"for(var i = 0 ; i < 10 ; ++i) {",
239-
getInstrumentCodeLine("C", 1, 0), ";",
270+
getInstrumentCodeLine("C", 1, 0), ";",
240271
" console.log('*');",
241-
"}",
242-
getInstrumentCodeLine("E", 1, 0), ";");
272+
"}");
243273

244274
String[] expectedArr = {instrumentCodeExpected, expected};
245275
test(options, sourceArr, expectedArr);
@@ -262,10 +292,10 @@ public void testSwitchInstrumentation() {
262292
lines(
263293
"switch (x) {",
264294
" case 1: ",
265-
getInstrumentCodeLine("C", 2, 3), ";",
295+
getInstrumentCodeLine("C", 2, 3), ";",
266296
" x = 5;",
267297
" default: ",
268-
getInstrumentCodeLine("E", 1, 0), ";",
298+
getInstrumentCodeLine("E", 1, 0), ";",
269299
"};");
270300

271301
String[] expectedArr = {instrumentCodeExpected, expected};
@@ -284,15 +314,15 @@ public void testNestedIfInstrumentation() {
284314
String expected =
285315
lines(
286316
"if (tempBool) {",
287-
getInstrumentCodeLine("C", 1, 0), ";",
317+
getInstrumentCodeLine("C", 1, 0), ";",
288318
" if (someBool) {",
289-
getInstrumentCodeLine("C", 1, 14), ";",
319+
getInstrumentCodeLine("C", 1, 14), ";",
290320
" console.log('*');",
291321
" } else {",
292-
getInstrumentCodeLine("E", 1, 14), ";",
322+
getInstrumentCodeLine("E", 1, 14), ";",
293323
" }",
294324
"} else {",
295-
getInstrumentCodeLine("E", 1, 0), ";",
325+
getInstrumentCodeLine("E", 1, 0), ";",
296326
"}");
297327

298328
String[] expectedArr = {instrumentCodeExpected, expected};
@@ -328,9 +358,13 @@ public void testInstrumentationMappingIsCreated() {
328358
.isEqualTo("AAA");
329359
}
330360

331-
private String getInstrumentCodeLine(String encodedParam, int lineNo, int colNo){
332-
return "module$exports$instrument$code.instrumentCodeInstance.instrumentCode(\"" +
333-
encodedParam + "\", " + lineNo + ", " + colNo + ")";
361+
private String getInstrumentCodeLine(String encodedParam, int lineNo, int colNo) {
362+
return Strings.lenientFormat(
363+
"%s(\"%s\", %s, %s)",
364+
"module$exports$instrument$code.instrumentCodeInstance.instrumentCode",
365+
encodedParam,
366+
lineNo,
367+
colNo);
334368
}
335369

336370
@Override

0 commit comments

Comments
 (0)