Fix #3250 - Makes form wizard the default #3252

karlcow · 2020-03-24T02:25:26Z

This PR fixes issue #3250

Proposed PR background

My feeling is that if we are out of the experimental phase, we probably need to just not go through the AB code at all, and all of it becomes a lot simpler.

We can keep in place the code for future AB experiments and open a new issue for removing the old form code and tests once @ksy36 has finished refactoring the rest of the code for the new form.

So on this hypothesis, let's create a new PR. I expect tests to break badly. That's an Achilles' heel of our current setup. The AB experiment was an experiment at the beginning. Never let an experiment grows without the proper setup. We need to fix it.

(env) ~/code/webcompat.com % pytest
============================= test session starts ==============================
platform darwin -- Python 3.7.4, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/karl/code/webcompat.com
collected 157 items

tests/unit/test_api_urls.py ...........                                  [  7%]
tests/unit/test_config.py ..                                             [  8%]
tests/unit/test_console_logs.py .....                                    [ 11%]
tests/unit/test_form.py .................                                [ 22%]
tests/unit/test_helpers.py ............................                  [ 40%]
tests/unit/test_http_caching.py ...                                      [ 42%]
tests/unit/test_issues.py ......                                         [ 45%]
tests/unit/test_rendering.py ......                                      [ 49%]
tests/unit/test_tools_changelog.py ..                                    [ 50%]
tests/unit/test_topsites.py ...                                          [ 52%]
tests/unit/test_uploads.py ...                                           [ 54%]
tests/unit/test_urls.py .................................                [ 75%]
tests/unit/test_webhook.py ......................................        [100%]

============================= 157 passed in 21.28s =============================

Unit testing seems to work. There was a simple fix. The big breakage should happen on the functional tests side. Let's create a PR and see how much CircleCI is not happy about it.

karlcow · 2020-03-24T02:54:40Z

No unit test coverage for chrome 75.0.3770.100 on Linux
chrome 75.0.3770.100 on Linux: 99 passed, 2 failed
TOTAL: tested 2 platforms, 198 passed, 4 failed
Stopping
Stopped
something bad happened inside intern.run()
{ Error: One or more tests failed
    at /home/circleci/repo/node_modules/intern/lib/executors/Executor.js:386:45
    at /home/circleci/repo/node_modules/@theintern/common/index.js:16:7174
    at process._tickCallback (internal/process/next_tick.js:68:7) reported: true }

Exited with code exit status 1

This test

127.0.0.1 - - [24/Mar/2020 02:28:50] "GET /issues/new HTTP/1.1" 200 -

× firefox on linux 4.15.0-1052-aws - Reporting (non-auth) - Description validation (1.113s)

    Error: [POST http://localhost:4444/wd/hub/session/97201fda-3d8f-4f01-8b55-252780618c70/element/b426a6ea-a2ab-4fcd-823f-bb9fbe157fc1/value / {"value":["b","u","g"," ","d","e","s","c","r","i","p","t","i","o","n"]}] Element <input id="description" class="form-field text-field required" name="description" type="hidden"> is not reachable by keyboard

    Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'

    System info: host: '7dea615d62c3', ip: '192.168.144.3', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-1052-aws', java.version: '11.0.3'

    Driver info: driver.version: unknown

      at Test.Description validation [as test] @ tests/functional/reporting-non-auth.js:148:12

      @ src/lib/Test.ts:266:51

and this test

127.0.0.1 - - [24/Mar/2020 02:28:52] "GET /issues/new HTTP/1.1" 200 -

× firefox on linux 4.15.0-1052-aws - Reporting (non-auth) - Submits are enabled (0.385s)

    Error: [POST http://localhost:4444/wd/hub/session/97201fda-3d8f-4f01-8b55-252780618c70/element/c729c5a0-fe29-4119-9ee9-42873bce6c36/value / {"value":["t","e","s","t"," ","f","o","r"," ","d","e","s","k","t","o","p"," ","s","i","t","e"]}] Element <input id="description" class="form-field text-field required" name="description" type="hidden"> is not reachable by keyboard

    Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'

    System info: host: '7dea615d62c3', ip: '192.168.144.3', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-1052-aws', java.version: '11.0.3'

    Driver info: driver.version: unknown

      at Test.Submits are enabled [as test] @ tests/functional/reporting-non-auth.js:226:12

      @ src/lib/Test.ts:266:51

Let's see if we can fix those two.

karlcow · 2020-03-24T03:29:07Z

ok fixed one test… maybe. circleci will tell us if we are off.

karlcow · 2020-03-24T04:33:16Z

Fixing the test will be painful.
Also why this is not working anymore.

It dies silently

node ./tests/functional/_intern.js "--functionalSuites=tests/functional/reporting-non-auth.js"

karlcow · 2020-03-24T05:52:30Z

🤔 puzzling

    NoSuchElement: [POST http://localhost:4444/wd/hub/session/27d4733c1552221fdb27c20e4cd4f712/element / {"using":"css selector","value":".step-1"}] 
no such element: Unable to locate element: {"method":"css selector","selector":".step-1"}

what do i do wrong?

karlcow · 2020-03-24T05:54:00Z

I wish i could run leadfoot inside the browser console.

karlcow · 2020-03-24T06:20:46Z

oh wait the new form is already tested in /tests/functional/reporting-issue-wizard-non-auth.js
ok let's remove the other one. and remove the cookie and if it fails, I guess it could be because of caching and that would explain why it was failing.

karlcow · 2020-03-24T06:35:58Z

ok this is more logical all tests are failing for the wizard form. so that means that the files are being cached.

× firefox on linux 4.15.0-1052-aws - Reporting with wizard - Wizard stepper - scenario 2 (10.254s)

    NoSuchElement: [POST http://localhost:4444/wd/hub/session/d1d9060f-fbfe-4f81-8c89-2314ac4dcb0d/element / {"using":"css selector","value":".button.step-1"}] Unable to locate element: .button.step-1

    For documentation on this error, please visit: https://www.seleniumhq.org/exceptions/no_such_element.html

    Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'

    System info: host: '66b5d67c4eeb', ip: '172.22.0.3', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-1052-aws', java.version: '11.0.3'

    Driver info: driver.version: unknown

      at Test.Wizard stepper - scenario 2 [as test] @ tests/functional/reporting-issue-wizard-non-auth.js:164:12

      @ src/lib/Test.ts:266:51

karlcow · 2020-03-24T07:28:29Z

I exhausted github through circleci for a little while. Let's wait.

ksy36 · 2020-03-24T18:03:47Z

I've checked out the branch, looks like it's not working because this condition is still in the templates (layout.html, new-issue.html):

{%- if ab_active('exp') == 'form-v2' %}

ksy36 · 2020-03-24T18:21:07Z

Also this should be removed? Probably no point of adding the label if all reports will be going through the new form

webcompat.com/webcompat/helpers.py

Lines 763 to 768 in fdf8bef

    
           if ab_active('exp') == 'form-v2': 
        
               if isinstance(extra_labels, list): 
        
                   extra_labels.append('form-v2-experiment') 
        
               else: 
        
                   extra_labels = ['form-v2-experiment']

webcompat/views.py

karlcow · 2020-03-24T21:53:16Z

yes the labeling should be removed. Good point.
{%- if ab_active('exp') == 'form-v2' %} and still manually I receive the right thing, but yes I will check this too. Good call. Thanks.

karlcow · 2020-03-25T04:06:16Z

ok this is much better now that I have fixed my issue with #3255 and a couple of other things.

karlcow · 2020-03-25T04:45:21Z

These are the tests failing

Image Uploads (non-auth) - Selecting an invalid image after a valid one
Image Uploads (non-auth) - Image extension validation
Reporting (auth) - Report button shows name
Reporting through postMessage - postMessaged object

karlcow · 2020-03-25T07:46:50Z

Only two tests to fix, and then we are good to rebase a bit the things and ride. :)

karlcow · 2020-03-26T03:01:35Z

ok all green locally. now let's rebase this. :)

Our AB testing experiment is finished. We could remove all this code or just make it more abstract for the next experiment we would want to do. So in this commit I removed the mention of form.

I'm not sure this test is fully neede or at least not in this form. There might be room for a more generic tests related to if there is an AB experiment going on and we choose to add a label, how do we track and test this.

some tests have been removed because duplicated elsewhere. some partially rewritten. The new form is definitely more challending to test than the previous one and there is a lot of repeat. In fact I would probably see in the future a way where we can branch scenarios in the tests through meta function, instead of repeating the same code again and again.

Instead of keeping the choice in between the old form and the wizard form, we make the wizard form the default. we should create another issue to finally remove all the code related to the old form.

karlcow · 2020-03-26T03:26:08Z

@ksy36 This is request for another review. the code changed quite a bit and i modified things with your recommendations.

@miketaylr I decided to make the wizard form the default.

also each commit as an extended description explaining the line of thoughts.

miketaylr · 2020-03-26T15:44:23Z

config/__init__.py

@@ -226,25 +225,23 @@ def get_variation(variation_key, variations_dict, defaults_dict):

 # Get AB experiment variation values from the environement.
 AB_VARIATIONS = {
-    'FORM_V1_VARIATION': os.environ.get('FORM_V1_VARIATION', '0 100'),
-    'FORM_V2_VARIATION': os.environ.get('FORM_V2_VARIATION', '100 100'),
+    'V1_VARIATION': os.environ.get('V1_VARIATION', '0 100'),


@karlcow can you make sure to update the environment variables on the server to match this?

(i did this)

miketaylr · 2020-03-30T18:35:04Z

I think we're good to merge and deploy (to staging first).

karlcow requested a review from ksy36 March 24, 2020 02:25

ksy36 requested changes Mar 24, 2020

View reviewed changes

webcompat/views.py Show resolved Hide resolved

karlcow added 4 commits March 26, 2020 12:15

Issue webcompat#3250 - makes AB generic

52582f7

Our AB testing experiment is finished. We could remove all this code or just make it more abstract for the next experiment we would want to do. So in this commit I removed the mention of form.

Issue webcompat#3250 - makes the wizard form the default

136e292

Instead of keeping the choice in between the old form and the wizard form, we make the wizard form the default. we should create another issue to finally remove all the code related to the old form.

karlcow force-pushed the 3250/1 branch from fdf8bef to 136e292 Compare March 26, 2020 03:24

karlcow requested a review from ksy36 March 26, 2020 03:24

issue webcompat#3250 - fixes style

36cbbf9

miketaylr reviewed Mar 26, 2020

View reviewed changes

ksy36 approved these changes Mar 26, 2020

View reviewed changes

miketaylr mentioned this pull request Mar 27, 2020

Fixes #3254 - Switch over to JSON config for Intern #3260

Merged

miketaylr merged commit 476c9c7 into webcompat:master Mar 30, 2020

miketaylr added the status: add to changelog label Mar 30, 2020

miketaylr removed the status: add to changelog label Apr 10, 2020

karlcow deleted the 3250/1 branch August 24, 2020 04:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3250 - Makes form wizard the default #3252

Fix #3250 - Makes form wizard the default #3252

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

ksy36 commented Mar 24, 2020

ksy36 commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 25, 2020

karlcow commented Mar 25, 2020 •

edited

karlcow commented Mar 25, 2020

karlcow commented Mar 26, 2020 •

edited

karlcow commented Mar 26, 2020

miketaylr Mar 26, 2020

miketaylr Mar 30, 2020

miketaylr commented Mar 30, 2020

Fix #3250 - Makes form wizard the default #3252

Fix #3250 - Makes form wizard the default #3252

Conversation

karlcow commented Mar 24, 2020

Proposed PR background

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 24, 2020

ksy36 commented Mar 24, 2020

ksy36 commented Mar 24, 2020

karlcow commented Mar 24, 2020

karlcow commented Mar 25, 2020

karlcow commented Mar 25, 2020 • edited

karlcow commented Mar 25, 2020

karlcow commented Mar 26, 2020 • edited

karlcow commented Mar 26, 2020

miketaylr Mar 26, 2020

Choose a reason for hiding this comment

miketaylr Mar 30, 2020

Choose a reason for hiding this comment

miketaylr commented Mar 30, 2020

karlcow commented Mar 25, 2020 •

edited

karlcow commented Mar 26, 2020 •

edited