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

Ow karate #3956

Closed
wants to merge 17 commits into from
Closed

Ow karate #3956

wants to merge 17 commits into from

Conversation

rahtr
Copy link
Contributor

@rahtr rahtr commented Aug 9, 2018

This PR consists of functional test cases using Karate BDD Framework.

Description

This PR intends to complement the existing functional tests cases using the Karate BDD framework. The idea of implementing this framework is to enable testers/QA to create complete E2E scenarios utilizing the common testing framework. Also as this Framework supports Gatling, it enables to create Smoke tests based on performance.

Related issue and scope

In process of creating brief wiki explaining about this approach in here-->https://cwiki.apache.org/confluence/display/OPENWHISK/OpenWhisk+Project+Wiki

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@csantanapr
Copy link
Member

there are files that I think should be git ignore, like .classpath, .settings/, .project/

test.txt Outdated Show resolved Hide resolved
tests/functional/karate_tests/LICENSE Outdated Show resolved Hide resolved
tests/functional/karate_tests/README.md Outdated Show resolved Hide resolved
tests/functional/karate_tests/README.md Outdated Show resolved Hide resolved
tests/functional/karate_tests/README.md Outdated Show resolved Hide resolved
tests/functional/karate_tests/src/test/test.iml Outdated Show resolved Hide resolved
@chetanmeh
Copy link
Member

You can drop the gradle wrapper related files as they are already part of core repo. So files like graddle-wrapper.jar, gradle.properties, gradlew.bat etc can be removed. Further add this module in settings.gradle in base directory.

Copy link
Member

@chetanmeh chetanmeh left a comment

Choose a reason for hiding this comment

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

We can cut down on some boiler plat code to reduce the code further and thus avoid the noise

@chetanmeh
Copy link
Member

Given Karate guidance on naming convention it would be better to change package name to just whisk. It would reduce the noise in feature file while referring other files

@rabbah
Copy link
Member

rabbah commented Feb 9, 2019

What's the plan for this PR - is there support to accept it and maintain it?

@rahtr
Copy link
Contributor Author

rahtr commented Feb 9, 2019

We are still using the Karate based tests and maintaining this repo-->(https://github.com/rahulqelfo/ow-karate).
We can think of having the Karate based tests within the OW repo or as a standalone project but IMHO,I think additional tests/test strategy is always a good to have.
If we plan to have it within the OW repo ,I can revisit and update this PR.

@codecov-io
Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #3956 into master will increase coverage by 21.61%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3956       +/-   ##
===========================================
+ Coverage   59.35%   80.97%   +21.61%     
===========================================
  Files         162      163        +1     
  Lines        7585     7594        +9     
  Branches      500      502        +2     
===========================================
+ Hits         4502     6149     +1647     
+ Misses       3083     1445     -1638
Impacted Files Coverage Δ
...penwhisk/core/containerpool/ContainerFactory.scala 85.71% <0%> (ø) ⬆️
...ala/org/apache/openwhisk/common/ConfigMXBean.scala 100% <0%> (ø)
.../apache/openwhisk/core/controller/Controller.scala 84.04% <0%> (+0.17%) ⬆️
...la/org/apache/openwhisk/core/invoker/Invoker.scala 71.66% <0%> (+0.48%) ⬆️
.../org/apache/openwhisk/core/entity/EntityPath.scala 100% <0%> (+1.88%) ⬆️
...a/org/apache/openwhisk/core/entity/Parameter.scala 95.45% <0%> (+2.27%) ⬆️
.../org/apache/openwhisk/http/PoolingRestClient.scala 90% <0%> (+3.33%) ⬆️
...hisk/core/controller/actions/SequenceActions.scala 92.1% <0%> (+3.5%) ⬆️
...pache/openwhisk/core/containerpool/Container.scala 88.6% <0%> (+3.79%) ⬆️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala 4% <0%> (+4%) ⬆️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56dca59...bbc33a4. Read the comment docs.

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

I went through this PR. Can we avoid some some of the duplication - like redundant gradle, some test artifacts, and I think we have a fileutil writer somewhere as well.

My concern with this PR is that it raises the question of where does one add tests when adding new functionality - existing scala tests or karate tests? Do we really get 20% extra coverage from these tests -- I thought we were in the 80s before.

tests/functional/karate_tests/README.md Outdated Show resolved Hide resolved

### How to add a new test type
1. Create a package in `src/test/java`.
2. We can create test types like regression, sanity etc.For example `org.apache.openwhisk.sanitytests`
Copy link
Member

Choose a reason for hiding this comment

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

space after etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved this in the latest commit.



### Config Variable(Optional) to run the tests
By default the tests would run using guest namespace.The variables in karate.config
Copy link
Member

Choose a reason for hiding this comment

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

using the guest namespace.
space before The.

Copy link
Contributor Author

@rahtr rahtr Feb 22, 2019

Choose a reason for hiding this comment

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

Resolved this in the latest commit.

3. Create a feature and runner file inside the above package


### Config Variable(Optional) to run the tests
Copy link
Member

Choose a reason for hiding this comment

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

space before (Optional)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved this in the latest commit.

@@ -0,0 +1,8 @@
# Licensed to the Apache Software Foundation (ASF) under one or more contributor
Copy link
Member

Choose a reason for hiding this comment

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

why do we need another 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.

Resolved this in the latest commit.

@@ -0,0 +1,45 @@
function() {
// Licensed to the Apache Software Foundation (ASF) under one or more contributor
Copy link
Member

Choose a reason for hiding this comment

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

license should come at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not supported in Karate. The file must start with function .


if (!adminauth) {

adminauth='d2hpc2tfYWRtaW46c29tZV9wYXNzdzByZA=='
Copy link
Member

Choose a reason for hiding this comment

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

should we hardcode anything in here?
might be better to just abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved this in the latest commit.

@@ -0,0 +1,8 @@
function() {
Copy link
Member

Choose a reason for hiding this comment

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

can't reuse existing test artifacts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the limitation as mentioned above, the existing artifact can't be re-used .Also the project is designed to be completely isolated from the existing ScalaTest and OW code. The idea is to have some sort of distinction from the existing tests

@rahtr
Copy link
Contributor Author

rahtr commented Feb 22, 2019

I went through this PR. Can we avoid some some of the duplication - like redundant gradle, some test artifacts, and I think we have a fileutil writer somewhere as well.

My concern with this PR is that it raises the question of where does one add tests when adding new functionality - existing scala tests or karate tests? Do we really get 20% extra coverage from these tests -- I thought we were in the 80s before.

The idea of this project is to introduce another dimension of testing(User Behaviors/Paths) which would complement the existing ScalaTests. So, the ScalaTests would serve more of a Smoke/Sanity/Integrations Tests at the system level and Karate tests would capture user based scenarios.
The community members(specially developers) will keep on adding the Unit/System/Integration tests using Scala and Testers can capture and add user based test scenarios in Karate.
This would definitely help in increasing the overall quality (if not coverage) as Karate Tests intend to cover the depth.
Also we can keep the Karate as optional tests.

@tysonnorris
Copy link
Contributor

@rahulqelfo Do you want to shepherd this PR further? Or see if Manoj will take it up? Otherwise, let's close for now, and reopen later if a new owner emerges. WDYT?

@rahtr
Copy link
Contributor Author

rahtr commented Apr 12, 2019

@rahulqelfo Do you want to shepherd this PR further? Or see if Manoj will take it up? Otherwise, let's close for now, and reopen later if a new owner emerges. WDYT?

@tysonnorris The PR has already been updated based on the responses from the reviewers. The only pending item is a decision to merge this and keep Karate Tests within the OW code base.

I can look at the jenkins CI failure, once someone can review this PR and suggest if further changes are required. Please suggest.

Configuration config = new Configuration(new File("target"), "openwhisk");
ReportBuilder reportBuilder = new ReportBuilder(jsonPaths, config);
reportBuilder.generateReports();
String dir = System.getProperty("user.dir");
Copy link
Member

Choose a reason for hiding this comment

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

looks like this line and the next are not needed.

"""
if(responseStatusCode==200){
karate.log("Action got deleted");
}
Copy link
Member

Choose a reason for hiding this comment

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

indentation minus 3? (here and few other places in this file)

tests/functional/karate_tests/.gitignore Show resolved Hide resolved
@rabbah
Copy link
Member

rabbah commented Apr 16, 2019

@rahulqelfo @tysonnorris I have no strong opinions either way on this PR as long as there's a commitment to maintain these tests as part of the repo. It would be useful to articulate when new tests should be added to this suite and if there's a long term vision for more coverage.

@tysonnorris
Copy link
Contributor

@rahulqelfo Please chime in on your plans to maintain and extend these tests, otherwise I think we should close this PR. At this point I don't think anyone at Adobe will maintain them, so I think this is up to you. I will verify with Manoj as well, in case he wants to take this up.

@rahtr
Copy link
Contributor Author

rahtr commented Apr 19, 2019

@tysonnorris , I agree that we can close this for now, as I would not be able to devote time on this PR at this moment. We can open it when we need it or find any owner for it.

@dgrove-oss
Copy link
Member

Closing per comment on April 18 above.

@dgrove-oss dgrove-oss closed this Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants