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

Feature/sjs perf #789

Merged
merged 4 commits into from
Mar 16, 2018
Merged

Feature/sjs perf #789

merged 4 commits into from
Mar 16, 2018

Conversation

paxtonhare
Copy link
Contributor

enjoy!

@paxtonhare paxtonhare requested a review from aebadirad March 9, 2018 21:33
plugins {
id 'net.saliman.properties' version '1.4.6'
id 'com.marklogic.ml-data-hub' version '2.0.4'
// id 'com.marklogic.ml-data-hub' version '2.0.4'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not have checked in.. will fix

import org.gradle.api.tasks.TaskAction

import com.marklogic.appdeployer.AppConfig
class MlcpTask extends JavaExec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this part of ml-gradle? from Rob's wiki I wouldn't think this would be needed (but I'm still learning about gradle)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he hasn't updated to the latest one yet. I forgot to put a comment in there about it. will do that.


let inputs = context.inputs
for (let key in inputs) {
tracelib.setPluginInput(key, inputs[key]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (obj.hasOwnProperty(prop)) { needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope. that blows up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant inputs.hasOwnProperty. Only relevant if inputs might inherit properties from somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. I fixed it to the right thing and it still blew up.

}

let cache = func();
//require.call(caller, moduleUri);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented out line

}
else {
const x = new NodeBuilder();
return x.toNode();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning here means the following lines won't be run. Looks like return x.toNode(); should be after the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like an edge case that would break if it were properly tested. I'll fix that.

}
}
else {
const x = new NodeBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer nb to x

traceSteps.push({
label: getPluginLabel(currentTrace),
input: getPluginInput(currentTrace),
error: rfc.isJson() ? error : error,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if pushing the same thing either way, why the test?

return tracelib.getTrace(id);
}

function getTraceIds(q) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is q a query? Either a comment or a better label would be useful

@@ -187,6 +208,7 @@ private JsonNode validateUserModules() {
DataHub dataHub = getDataHub();
List<DynamicTest> tests = new ArrayList<>();

// flows before a structured change get updated. this ru
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment missing part of the sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to remove it because I have no clue what I was typing there.

FinalCounts finalCounts = new FinalCounts(0, 0, 1, 1, 0, 0, 0, 0, 0, 0, "FAILED");
testInputFlowViaMlcp(prefix, "-2", stagingClient, codeFormat, dataFormat, useEs, options, finalCounts);
}));
// tests.add(DynamicTest.dynamicTest(flowName + ": " + plugin + " error MLCP", () -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove outdated code

added tests for missing cases
@marklogic-builder
Copy link

Jenkins Build failure
https://jenkins.marklogic.com/job/Labs/job/DHF_b9_nightly_linux_cluster/196/testReport
453 tests run, 0 skipped, 153 failed.

@paxtonhare paxtonhare merged commit 4fe2445 into develop Mar 16, 2018
@paxtonhare paxtonhare deleted the feature/sjs_perf branch March 16, 2018 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants