You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by guicaro <gi...@git.apache.org> on 2017/03/15 16:23:08 UTC
[GitHub] zeppelin pull request #2141: [ZEPPELIN-1720] Adding tests to verify behaviou...
GitHub user guicaro opened a pull request:
https://github.com/apache/zeppelin/pull/2141
[ZEPPELIN-1720] Adding tests to verify behaviour of dynamic forms
### What is this PR for?
Adding Selenium tests to ensure proper behaviour of dynamic forms.
### What type of PR is it?
Test
### Todos
N/A
### What is the Jira issue?
[ZEPPELIN-1720](https://issues.apache.org/jira/browse/ZEPPELIN-1720)
### How should this be tested?
1. Once should first get Firefox v. 41 as it is the latest version that works with the current version of Selenium in Apache Zeppelin.
2. You can then run the tests with following command:
`TEST_SELENIUM="true" mvn package -DfailIfNoTests=false -pl 'zeppelin-interpreter,zeppelin-zengine,zeppelin-server' -Dtest=ParagraphActionsIT
`
### Screenshots (if appropriate)
N/A
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/guicaro/zeppelin ZEPPELIN-1720-AddingTestsForDynamicForms
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/2141.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2141
----
commit 17ca94208f5b4553e3cc2a93682b6baa0f626457
Author: Guillermo Cabrera <gu...@gmail.com>
Date: 2017-03-13T16:27:22Z
Added method stubbs for new UI tests checking correct behaviour of dynamic forms
commit e6bcd78edf9197b13c0ec12e7e03712d895bc4c9
Author: Guillermo Cabrera <gu...@gmail.com>
Date: 2017-03-14T12:50:01Z
Completed and verified corrct behaviour of testSingleDynamicFormTextInput
commit 5507be53894622329208cac8511103f2031fc6a8
Author: Guillermo Cabrera <gu...@gmail.com>
Date: 2017-03-14T16:50:26Z
Added tests that cover behaviour of dynamic forms
commit ce0823db0d3189569131208ab16a0ff8751d0762
Author: Guillermo Cabrera <gu...@gmail.com>
Date: 2017-03-15T15:23:57Z
Removed unused import, alignment and removed unnecesary condition in test case
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Cool!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by Leemoonsoo <gi...@git.apache.org>.
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro Thanks for the contribution!
CI problem has been fixed on master branch. Could you try rebase or merge master branch and see if CI becomes green?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Merge into master and branch-0.7 if there are no more comments on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro Thanks for your great contribution!
I left some comments. Could you take a look at them? Please let me know if there is something that I missed :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Quick question, what is the behavior when we have more than 2 select forms? Shouldn't we also have a **"Run on selection"** check box? I believe this was one of the pain points for some (see [ZEPPELIN-1013](https://issues.apache.org/jira/browse/ZEPPELIN-1013)). Here is a screen shot showing a paragraph with 2 forms created programmatically but not showing the drop down menus nor the 'Run on selection" checkbox:
Maybe I'm missing something? I rebased yesterday from master and have changes up to March 17 on [my fork ](https://github.com/guicaro/zeppelin/commits/master)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@1ambda Thanks, I actually did setup Travis while I was developing, I did not realize that every commit triggered a build so I deactivated my Zeppelin fork on Travis after I noticed this (so only have 2 builds on Travis). I activated it before I got this PR, so now I am in a state where I don't have a build for my latest commit and this PR does not show up on Travis. Should I trigger a manual build of last commit via the Travis API? Is there any other way to get back on track?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro I see. There are many ways to trigger travis again. but what you actually need is pushing new commits so that travis get noticed.
- Amend your last commit with `git commit --amend` and `push --force`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin pull request #2141: [ZEPPELIN-1720] Adding tests to verify behaviou...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zeppelin/pull/2141
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Seems some review comments are mixed up in github side :(
I just removed some of them and left comment again. Please let me know if you're confused.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro There are some unrelated commits in here after you rebased. Can you try to fix it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro Thanks for letting me know. Let me check again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Thanks again @1ambda it all worked like a charm and now all green :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Selenium tests are red in Travis but due to:
`SparkParagraphIT.testSqlSpark:185 � NoSuchElement Unable to locate element: {"... `
https://travis-ci.org/guicaro/zeppelin/jobs/212973588
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@AhyoungRyu I'm currently looking at why testMultipleDynamicFormsSameType is failing. It now fails after I rebased from master. It's the test case that includes two select forms in one single paragraph. Basically the select foms are not showing up in GUI. I tested this with new version of Chrome (Version 56.0.2924.87 (64-bit)) to see if this might be an issue with the old Firefox version we use for Selenium, but also happens in Chrome. Here is the pic:
You can reproduce by entering the following string into a paragraph and then running (this used to work before I rebased).
`%spark val options = Seq(("han","Han"), ("leia","Leia"), ("luke","Luke")); println("Greetings "+z.checkbox("skywalkers", options).mkString(" and "))`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@AhyoungRyu Fixed the issue with my commits, big shoutout to @DrIgor for helping resolve my issue! I also updated the tests (ex. `testSingleDynamicFormSelectForm`, `testSingleDynamicFormCheckboxForm`, `testMultipleDynamicFormsSameType`) to work with the new paragraph behaviour. All 14 tests pass now with the latest code in master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by guicaro <gi...@git.apache.org>.
Github user guicaro commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@AhyoungRyu no worries, will be glad to review your work, fix `testMultipleDynamicFormsSameType` and fox my commits, will definetly ping you about this last issue :) Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by AhyoungRyu <gi...@git.apache.org>.
Github user AhyoungRyu commented on the issue:
https://github.com/apache/zeppelin/pull/2141
@guicaro I feel so sorry to you since it was my bad. Just created hotfix patch #2154 to fix below two issues that you reported.
- Select form is not shown up when multiple dynamic forms (e.g. 2 select forms / 1 checkbox + 1 selectform) are in one paragraph
- Can't see `Run on selection change` menu under the paragraph control menu
It'll be appreciated if you can help me out to review #2154!
BTW I tested all your test cases on my patch, most of them passed except `testMultipleDynamicFormsSameType`. It's because of [this line](https://github.com/apache/zeppelin/pull/2141/files#diff-48409cf8ca5ddb1aea1a62ada3cd091dR649). The expected matcher should be `"Howdy 1\nHowdy "` instead of `"Howdy \nHowdy "` since the paragraph runs right after the selection has been changed. (as I said before :D)
And after you rebased, some irrelevant commits are mixed and pushed together in here. Could you handle them? Feel free to ping me if you're in trouble while you resolve it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2141: [ZEPPELIN-1720] Adding tests to verify behaviour of dy...
Posted by 1ambda <gi...@git.apache.org>.
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2141
Hi @randerzander. Thanks for contribution.
- Zeppelin build system relies on travis. So please setup your travis account and travis project for Zeppelin.
- https://zeppelin.apache.org/contribution/contributions.html#continuous-integration
- The repository name must be `zeppelin` (not `incubator-zeppelin`, ...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---