mozilla :: #testpilot

16 May 2017
00:54chuckJSON_voorhees: my care levels about that are pretty low, but I'd be happy to if I'd be helpful.
01:07JSON_voorheeschuck: heh alright, just thought i'd include you cus you commented on the bug. We can def figure it out tho :)
15:32fzzzygeez pontoon is spammy
15:40lorchardYeah, we shunted it over into a branch on Snooze Tabs, but it just defers the commit spam until merge
15:40lorchardGit doesn't really have a good way to squash a bunch of commits into one while retaining all the authors
15:46fzzzybut that would still cut down on the irc spam since there would only be one message for the merge, right?
15:47fzzzywe should do that
15:47lorchardOh, IRC spam is off on a side channel, so I don't care too much about that
15:47lorchardI'm more annoyed by wading through commit spam trying to see what happened on the project
15:48lorchardI think we still get IRC messages on branches though
15:48bwintonlorchard: `git log --first-parent`? :)
15:49lorchardDoesn't really help when I'm browsing through github for a quick link in an issue
15:49fzzzythe irc spam makes the side channel way less useful. if there were one irc spam per branch merge it would make the channel much more useful
15:49fzzzynot too big of a deal tho
15:50fzzzybecause there is one irc message per circle run for every pontoon commit too
15:51lorchardYeah, we might want to look at putting pontoon on a branch now that that's a possibility
15:52lorchardkind of weird that everything we do against master is subject to PRs & reviews & squashing, except pontoon
15:53fzzzywould cut down a lot on load on circle too
15:55fzzzyI made an issue for it https://github.com/mozilla/testpilot/issues/2430
15:55chuck#testpilot-pontoon-bots
15:55fzzzyhehe
15:56flodif you want Pontoon to commit in a branch, you also need to put the en-US new strings there (it might be obvious, or not, but worth a reminder)
15:56lorchardI actually kind of hate the bots channel... should be in the main channel, and turn them off if they're not something we want to hear from
15:56lorchardBut, meh
15:56fzzzyflod: ok
15:57fzzzylorchard: if the spam goes down enough having them on the main channel would be good
15:58fzzzylorchard: how did you manage what flod mentioned in snooze tabs?
15:58lorchardMerges go both ways
15:58lorchardMerge l10n to master, merge master to l10n periodically
15:58fzzzyk. it would be nice if it was automatic
15:58fzzzy(master -> l10n automatic, l10n -> master manual)
15:59lorchardWell, we wanted to be sure string churn was done before exposing to localizers
15:59fzzzybut that's for extra credit
15:59fzzzyI'm fine with manual both ways too
15:59lorchardand auto-merge l10n to master gets you the problem back that moving to a branch solved
15:59fzzzysomeone'll just have to remember to do it :)
15:59flodone advantage is that you can group strings when doing master -> l10n, and I could finally review them :-)
16:00fzzzyyeah that's a good advantage flod.
16:51_6a68lorchard|doctor | fzzzy: we could have a separate pontoon repo, and use git subtree or (ugh) submodules to bring in the latest
16:52_6a68I agree it's pretty noisy; the top 30 entries in my inbox today were test pilot dev addon build notices from AMO
16:52_6a68and I do miss the bots
16:52fzzzybranch seems simpler
17:09_6a68fzzzy: I think the conversation yesterday around TxP addon post-SDK came back to an embedded webextension, where the test pilot team provides telemetry and survey APIs in the embedding bootstrapped part, and individual experiments can manage API changes by choosing to update the npm package. Does that sound right to you?
17:09_6a68dcoates: ^^
17:09_6a68(Is anyone working on an SDK migration patch atm?)
17:09fzzzy_6a68: i think so
17:10fzzzyI'll be starting sdk migration on test pilot addon soon but have not yet started
17:14_6a68cool, so I guess we will need to figure out (1) how to smoothly migrate experiments from SDK or pure webextension to embedded (2) how to generate the bootstrapped wrapper as part of a build process, which will probably need to involve the new web-ext command line tool.
17:15_6a68fzzzy: Are there any other huge open questions related to the migration? ^^
17:27fzzzy_6a68: not that i can think of
17:45_6a68dcoates: any thoughts? you were talking about API experiments a bit yesterday
17:47_6a68fzzzy: have you thought about API experiments at all? the addons people were saying they could land a patch that hides mozilla-signed addons in 55, so we could put our test pilot code in an API experiment, and auto-install it when experiments are installed, if it isn't present
17:47dcoatesi do, but I should write up a doc for it so the nuance is clear
17:47jgruenfzzzy: https://github.com/mozilla/testpilot/issues/2434
17:48jgruenthere's too much state-change jank to do away with a the loading state for pages
17:48fzzzy_6a68: sure, we should explore it, but it seems hard to ensure the right pieces are installed with the right versions. is there any facility for installing multiple addons at once, or in the same xpi?
17:49fzzzyjgruen: :(
17:50jgruenyeah, it's just a lot of little cuts
17:51_6a68fzzzy: I think API versioning is an open question, but it otherwise is a lot simpler to use than an embedded bootstrapped addon, you just request the API in your manifest
17:52fzzzyjgruen: do you think it would ever be possible to fix them all?
17:52fzzzyi'll work on that before i start the sdk removal then
17:53jgruenmaybe...seems like a bit of a wild goose chase to me
17:53jgruensome seem easy, like we could give a fixed height to participant counts,
17:53jgruenbut i don't want to flash incorrect state
17:54fzzzy_6a68: how about we produce a proof of concept of each approach, then we can have more informed discussion?
17:54_6a68sounds great! dcoates ^^
17:54fzzzyjgruen: but you want a flash of loading bar? :)
17:55dcoatesfzzzy:
17:55jgruenfzzzy: uploading a video to vimeo to show the issue...i want the pages to come in all at once
17:55fzzzydcoates: what exactly is implemented in your proof of concept?
17:56fzzzyjgruen: i think what i am going to do is just render the loading indicator above the fold, and then just remove it once the js runs. this will have the best of both worlds.
17:56jgruenfzzzy: what constitutes the fold?
17:56jgruendifferent clients have different window
17:56jgruensizes
17:57fzzzyjgruen: it'll just take up all the visible horizontal space
17:57dcoatesfzzzy: for now just an api stub and a button. i can add a versioning strawman as well
17:57fzzzyjgruen: thanks for providing such detailed feedback
17:58fzzzydcoates: k i'll take a more detailed look later this week. thanks!
17:59jgruenfzzzy: that seems more complicated to try to selectively mask off content, what do you mean by "best of both worlds"
17:59jgruenalso https://vimeo.com/217709276
18:00fzzzyjgruen: it won't mask content, it'll just push it down the page
18:00fzzzyi'll just implement it and you can see if you like it :)
18:00jgruensgtm
18:00fzzzyit won't take long, i had an implementation like that before
18:00jgruenfzzzy, i filed a separate but regarding installed users rerouting to /experiments
18:01fzzzyjgruen: yeah, we can do that while the loading thing is visible
18:01jgruenthat's what i figured
18:01fzzzy
18:01jgruenFWIW, I'm happy we removed react router
18:02jgrueni just wanna make sure we resolve outstanding UI issues with plenty of time left in the sprint
18:03fzzzysure
18:03fzzzywe have lots of time since we extended the sprint :)
18:03fzzzyI'm very happy to polish
18:03fzzzyI'd love to learn if some of those problems can be eliminated by loading things in a different order or something, too
18:12jgruenfzzzy: best order here might be to bring back the loading module, then focus on better componentizing those stateful bits, then playing with how to render them more quickly/accurately without the dom flopping around
18:15fzzzyjgruen: I have the loading thing working already. seems pretty awesome.
18:18fzzzyjgruen: if you have time you can check it out and see if you like it: https://github.com/fzzzy/testpilot/tree/2434-reinstate-loading-indicator
18:36jgruenfzzzy: awesome
18:36jgrueni'll pull it down in a bit
19:08lorchardHmm, I think there's more going on here than the loading indicator not giving time
19:09fzzzylorchard: there's some redux state that is dispatched after the page renders, that is part of it
19:09lorchardAlso seeing some CSP errors rejecting the colors & images for experiment card - i.e. they *never* load
19:10fzzzyhmm, yeah, i guess csp isn't enforced locally but is on dev
19:10lorchardBut we didn't change any CSP rules, so I'm wondering what is differemnt
19:10lorchard(i.e. this worked before)
19:10fzzzyyeah doesn't seem like anything changed related to that
19:11lorchardUnless the remove react router changed it and I didn't notice, but I tried to comb for that kind of thing
19:11lorchardTook a screen grab of errors https://github.com/mozilla/testpilot/issues/2434#issuecomment-301885517
19:12fzzzyi see them
19:13lorchardHmm, I just see this https://www.dropbox.com/s/15jm0r1aw3amlwl/Screenshot%202017-05-16%2015.13.14.png?dl=0
19:17ianbickingsharon: stage is updated, and could host the tests
19:17fzzzylorchard: no, i mean i see the errors. i do not see the images
19:17lorchardOh okay
19:18lorchardMan, CSP seems so opaque and hard to decipher & debug :/
19:18lorchardalmost like regular expressions
19:18sharonianbicking: Cool, OK
19:26lorchardHuh, seems like we need `style-src 'unsafe-inline'` added to CSP. But as far as I can tell we've never had that in there on prod, so I don't know how this worked before
19:27lorchardAnd our CSP hasn't been changed for 6 months :/
19:28lorchardOnly thing I can think of is that somehow browsers are okay with inline styles if they belong to new DOM nodes from React and not DOM nodes parsed from server-delivered HTML
19:28lorchardbut that seems wrong
19:29lorchardand also I guess we should make local dev server include the CSP header to catch these issues :/
19:31fzzzylorchard: how did you test that? csp doesn't seem to apply locally
19:32lorchardI haven't tested it yet
19:32lorchardI'm in the process of adding a CSP header to the local server middleware
19:32fzzzyoh, ok
19:32lorchardJust a guess after staring at the CSP
19:32fzzzywhat makes you think unsafe-inline is required then?
19:32fzzzyk
19:32lorchardAnd also it's actually suggested by Google Chrome :)
19:33lorchardhttps://www.dropbox.com/s/xss5nodkv0e39pq/Screenshot%202017-05-16%2015.33.05.png?dl=0
19:33fzzzyheh
19:33lorchardJust remembered I tried it in there too
19:33fzzzyI don't think we can do a nonce because we don't have a dynamic server
19:34lorchardYeah, and we're not going to hash everything
19:34fzzzyit looks like it is only the experiment-icon-wrapper
19:34fzzzyit looks like it is only the experiment-icon-wrapper
19:35lorchardAlso the icon
19:35fzzzyyeah and experiment-icon
19:36fzzzyI guess the way react creates the nodes and adds the styles doesn't trigger the csp
19:36fzzzyseems like it should though
19:36lorchardYeah, which seems crazy
19:37lorchardsince one of the things CSP should be guarding against is dynamic node creation from XSS ert
19:37lorchards/ert/etc/
19:37fzzzyright
19:41fzzzymaybe because doing something like node.style.foo = 'bar' doesn't trigger the css parser?
19:49lorchardAh, yup I think that's what it is
19:49lorchardhttps://github.com/facebook/react/issues/5878#issuecomment-172964375
19:56lorchardThink I got a PR to fix it and add CSP to local dev
19:59fzzzylorchard: cool. i'm working on moving the css styles out of the yaml into the css styles.
19:59lorchardMight be better to maybe generate a separate CSS file *from* the YAML, so we can keep all the content in YAML
20:01fzzzyhmm. ok
20:01lorchardSince the colors & images are per-experiment
20:01* fzzzy nods
20:03lorchardOof, actually looks like we have a few spots with inline styles :/
20:04lorchardhttps://pastebin.mozilla.org/9021923
20:04lorchardBut maybe most of them are just client-side things
20:05fzzzysome of them might be ok if they aren't in the initial page render.
20:05fzzzyyeah, if they are just client-side things.
20:06fzzzyi'm not sure i see how i could generate one css file in gulp.task('content-experiments-data'
20:07lorchardYou could do it in the endStream() function, which also generates the FTL for all experiments
20:07lorchardand the JSON
20:08fzzzyok cool, thx
20:08lorchardcollectEntry() touches each experiment, endStream() wraps it all up
20:08lorchardSo you could accumulate the CSS rules & IDs or whatever in collectEntry() and then generate in endStream()
20:09lorchardheh man this gulp stuff is getting creaky
20:09fzzzyit looks like endStream runs for every experiment though?
20:09lorchardI do wonder if we could get cleaner switching to Jekyll or a JS-based site generator, but then I worry we have enough weird exceptions to funk one of those up too
20:10lorchardIt shouldn't
20:10fzzzyI guess that just wastefully creates intermediate files without all the experiments in them, then overwrites them
20:10lorchardThat should just be the function that gets called once, when the stream of files runs out
20:11fzzzyok, I see, buildExperimentsData only gets run once, but the return value gets run for every yaml file
20:11lorchardWell, the return value is a gulp Pipe
20:11fzzzyright, but each yaml file gets piped into it right?
20:11fzzzygulp is still fuzzy for me
20:11lorchardSo gulp.src() pipes all the YAML files through it. collectEntry() called for each of those, and then endStream() once the last one is through the pipe
20:12lorcharder gulp.pipe() that is
20:12fzzzygreat, that helps me understand what through.obj does a lot better, thanks.
20:13lorchardYeah, and endStream() is kind of a side effect, since collectEntry() uses this.push() to send items on to the next thing in the pipe gulp.dest()
20:14lorchardHmm, and actually I think that bit that generates usage_counts.json is dead code now too
20:14lorchardsince that's from before we had a source for them from Telemetry and we were faking them from the last good count from Django
20:16lorchardI guess we might be able to get away from generating individual api/experiments/{slug}.json files now, since we just cram the whole shebang into the JS bundle now
20:17fzzzyfile a bug! :)
20:23lorchardAlso poking some more at the integration test stuff in 2380 since last night and thoroughly baffled.
20:24lorchardThey work when I run them manually, but not when I let the test run go automatically. As far as I can tell, the gulp web server is running, but for some reason I don't understand the Firefox instance can't reach it
20:24lorchardI can even run curl before & after running tox, and things work as expected.
20:25fzzzyfirefox rejects the ssl cert maybe?
20:25lorchardWell, the error looks like it can't even make the network connection
20:25lorchardWith or without SSL enabled
20:26digitaraldshould minvid work on air.mozilla?
20:30JSON_voorheesdigitarald: nope
20:30JSON_voorheesIt would be great to add support eventually though
20:35fzzzylorchard: maybe the server needs to explicitly listen on 127.0.0.1?
20:35fzzzybut why would curl work then
20:35lorchardYeah, these are things baffling me
20:36lorchardTrying another run by changing the port, in case that's a thing :/ https://circleci.com/gh/lmorchard/testpilot/685
20:36lorchardTrying to figure out what the differences are, since once everything run & failed, I can ssh into that box and run the tests manually to see them pass
20:37lorchardBut whoami reports the same user during the automated failing run and my manual run. Trying to think what else could be different
20:38lorchardOh yeah, even try waiting 30 seconds for the hell of it in case there's something spinning up
20:44fzzzylorchard: removing the inline styles wasn't too bad https://github.com/fzzzy/testpilot/commit/789079030864aaba4b8f888acf7d3d9ef67bf16c
20:45fzzzyI guess I should make a WIP PR
20:45lorchardEh, yeah, that doesn't seem terrible
20:45lorchardMight tweak my PR to just include CSP in local dev and get rid of the unsafe-inline then
20:46fzzzylorchard: I already merged it, I'll undo the unsafe-inline in my pr and see if it works now
20:46lorchardAh, cool
21:05fzzzylorchard: r? https://github.com/mozilla/testpilot/pull/2437
21:26fzzzyjgruen: is there any possible way to fix the css font jank? if a page uses a css font it is always going to render in the system font, then redraw once the css font loads, i think. that sux
21:27jgruenwell, we could bundle the font face instead of calling it from a CMS?
21:27fzzzyseems like other people would have run into this issue
21:27jgruenthat's what FxA does
21:28jgruenthere are even services that looks through your strings and figure out how to optimize the character set in the face you load
21:28fzzzyheh ok good, i figured someone else must have run into this issue
21:36fzzzyif anyone is up for a four line review: https://github.com/mozilla/testpilot/pull/2439
21:51_6a68merged
22:19clouserwjgruen: no plug for min vid in that email?!
17 May 2017
No messages
   
Last message: 99 days and 5 hours ago