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

Fixing issue #1451 #1454

Merged
merged 7 commits into from
Jan 28, 2021

Conversation

joelpramos
Copy link
Contributor

@joelpramos joelpramos commented Jan 27, 2021

Description

Thanks for contributing this Pull Request. Make sure that you submit this Pull Request against the develop branch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.

@joelpramos
Copy link
Contributor Author

Updated upload.feature due to failing unit test.

image

Some food for thought is that HTTP headers are not case sensitive although trying to come up with special handling just for that might be too painful and might as well leave as is - it's a testing framework so results should be repetitive anyways.

@joelpramos joelpramos marked this pull request as draft January 27, 2021 01:15
@joelpramos
Copy link
Contributor Author

The same scenario gave me a different response the second time around? I'm logging off but maybe can check it out in the morning.

image

@ptrthomas
Copy link
Member

the build failed and what's weird is the error message is kind of lacking, maybe this is the header code path not good enough, do investigate if you can
image

@joelpramos
Copy link
Contributor Author

the build failed and what's weird is the error message is kind of lacking, maybe this is the header code path not good enough, do investigate if you can
image

yeah I'm looking at it and it seems like couple things - the headers are not consistent upper case or lower case even in my local in IntelliJ runner (like click the button) vs. running with Maven and then here with the CICD pipeline. I think might need to make a check case insensitive (like match Content-Type or match content-type #notnull)

Also getting this sometimes so I assume it's my laptop that's super slow (not typically a coder here..). Are you cool if I increase the timeout for the testing? I assume that's somewhere in a karate-config.js
image

@ptrthomas
Copy link
Member

Also getting this sometimes so I assume it's my laptop that's super slow (not typically a coder here..). Are you cool if I increase the timeout for the testing? I assume that's somewhere in a karate-config.js

yes go ahead

@joelpramos joelpramos marked this pull request as ready for review January 28, 2021 00:39
@ptrthomas ptrthomas merged commit 6e3f651 into karatelabs:develop Jan 28, 2021
@joelpramos
Copy link
Contributor Author

Also getting this sometimes so I assume it's my laptop that's super slow (not typically a coder here..). Are you cool if I increase the timeout for the testing? I assume that's somewhere in a karate-config.js

yes go ahead

@ptrthomas I noticed the CI/CD build failed after you merged to develop but it was timeout as well. I ended up not pushing the change and keeping it in my local FYI.

2021-01-28T12:50:47.1608727Z org.graalvm.polyglot.PolyglotException: http call failed after 10 milliseconds for url: https://127.0.0.1:41913/headers
2021-01-28T12:50:47.1610980Z classpath:demo/headers/common-noheaders.feature:8
2021-01-28T12:50:47.1613639Z - com.intuit.karate.core.FeatureResult.getErrorMessagesCombined(FeatureResult.java:215)
2021-01-28T12:50:47.1617580Z - com.intuit.karate.core.ScenarioEngine.callFeature(ScenarioEngine.java:1938)
2021-01-28T12:50:47.1620243Z - com.intuit.karate.core.ScenarioEngine.call(ScenarioEngine.java:1854)
2021-01-28T12:50:47.1622871Z - com.intuit.karate.core.ScenarioBridge.callSingle(ScenarioBridge.java:209)
2021-01-28T12:50:47.1624045Z - <js>.fn(Unnamed:21)
2021-01-28T12:50:47.1624297Z 

@ptrthomas
Copy link
Member

@joelpramos argh what do you suggest, do I revert this ?

@joelpramos
Copy link
Contributor Author

Don't think so, I think we should just increase the timeout, the failure doesn't seem to be related to the actual code fix. Originally it was breaking because of some different behavior in the web container so I just forced the headers to be lower case in this method: karate-demo/src/main/java/com/intuit/karate/demo/controller/SearchController.java

It just leaves me wonder whether my fix here is poor and it'll always hit the if condition whenever we're doing a match against something that is not present - however I can't think of a way to reduce the number of iterations for that piece of code. It'll keep it in the back of my mind to get back to it later tonight.

} else if (!optional && actual.isNotPresent()) {
    // if the element is not present the expected result can only be
    // the notpresent keyword, ignored or an optional comparison
    return expected.isNotPresent() || "#ignore".contentEquals(expected.getAsString());
}

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.

2 participants