mozilla :: #testpilot

11 Aug 2017
01:19fzzzywell, switching back to coda.app feels like putting on a favorite old pair of gloves that fits really well. and there's an ipad version too.
01:20fzzzythis may fix the 'e is null' error we're getting a lot of reports about on prod: https://github.com/mozilla/testpilot/pull/2766
01:20fzzzyI had a similar problem on my remove-user-counts branch and that fixed it. I wish we could repro the prod issue though, so weird that nobody can repro
04:57chuckIf anyone wants to check out a deployed Storybook: https://chuckharmston.github.io/testpilot-contribute
05:27fwiwanyone find that some container icons are too close to the border?
05:28fwiwlike this shopping one, http://i.imgur.com/LsOEaqV.png and the work one, http://i.imgur.com/FKsf0l1.png
15:01Caspy7Just wanted to mention that I found it annoying when the Test Pilot button with "NEW" popped up and after clicking on it, it did not go away, notably, the "NEW" label did not go away (at least immediately)
15:01Caspy7I'm on nightly
15:52lorchardCaspy7: Yeah, I think my implementation there is not quite right. Some other tweaks worth thinking about here too https://github.com/mozilla/testpilot/issues/2747#issuecomment-321634270
15:53lorchardit tracks the last time you clicked it... but, thinking through how it works, I think it waits until the next refresh of data from the site to make "new" disappear - which could be up to 4 hours
15:54Caspy7oof
15:54Caspy7lorchard: why not make NEW disappear on click?
15:54lorchardMostly because I didn't make it work that way :) no good reason
15:55lorchard*should* probably work that way, though. going to file an issue
15:58Caspy7
15:59lorchardhttps://github.com/mozilla/testpilot/issues/2769
16:00lorchardTrying to think if there are more cases to cover, feel free to comment!
16:11EeemspauloiegasSV: You're SoftVision-PaulOiegas on github right?
16:12pauloiegasSVEeems: hi, yes :)
16:12EeemsLet me just zip up my profile and share it with you. There is sensitive information in it.
16:12pauloiegasSVok
16:13EeemsHuh, getting permission errors while attempting to zip it
16:14EeemsThere we go, third time's the charm.
16:14pauloiegasSV:D
16:26Eeems@lorchard do you have containers enabled on your profile?
16:26Eeemsand/or did you have the containers experiment installed?
16:27lorchardYeah
16:27lorchardAlso tried with fresh profiles on 3 OSes & 3 release channels of Firefox
16:27EeemsSo the containers thing is probably a rabbit trail
16:28lorchardMy daily use profile has all test pilot experiments enabled just to make sure I'm a good test pilot :)
16:28lorchardWell, most of them anyway, since a few are mutually incompatible
16:29EeemsRight
16:29EeemspauloiegasSV just told me he was able to recreate it with my profile
16:31pauloiegasSVyes, sent lorchard the link on Slack private
16:31lorchardHmm, I usually don't get slack messages, will have to check
16:31pauloiegasSVchuck & lorchard please let Eeems know when you guys downlaoded it so he can invalidate the link
16:32chuckI'll have it in 2 minutes!
16:33chuckThanks for the help Eeems.
16:33Eeemsmy pleasure
16:35lorchardYeah I've got it too
16:35lorchardoh and hey, there are like a dozen private messages on slack I haven't seen til now
16:35Eeemshaha
16:36Eeemschuck: so you've got it now?
16:36chuckI do!
16:36lorchardNot sure what I'm doing wrong, but most of the time I check slack, I'm logged out
16:36Eeemsalright, invalidating the link
16:36pauloiegasSVby the way guys, disconnect Eeems Fx account before doing any changes
16:37Eeems^ yes please
16:38pauloiegasSVby the way, I have reproduced it by creating a new profile, deleting the new profile folder content and pasting E e e m s profile content in it.
16:38pauloiegasSVUntill now, I tried disabling all extensions and safe mode start, but this didn't changed anything
16:39pauloiegasSVso it might be something from about:config
16:44pauloiegasSVafter looking in browser console with everything disabled, this pops up constantly
16:44pauloiegasSVNS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIInterfaceRequestor.getInterface]
16:45pauloiegasSVnot sure if it's related. and Gijs already logged an issue for this 2 years ago https://bugzilla.mozilla.org/show_bug.cgi?id=1179250
16:45firebotBug 1179250 NEW, nobody@mozilla.org NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIInterfaceReques
16:45pauloiegasSVlol
16:46pauloiegasSVif it's related, maybe some details from the comments could help
16:47lorchardAh HAH, I can reproduce with the profile - and I can reproduce in my own profile if I go to my language options, remove en-US and add en-CA
16:47lorchardEeems: Thanks so much for the profile, tried carefully not to muck with anything of yours in there
16:48lorchardlooks like some sites logged in, but now I'm going to exit and delete the whole thing so as not to disturb anything
16:48Eeemslorchard: haha
16:48Eeemsthat's great
16:48EeemsI've broken a number of things by setting my language to en-CA
16:48lorchardThis also makes me think maybe fzzzy's PR might fix it, since he found an error in the l10n-related code
16:49lorchardsharing a full profile like that is a really generous thing, so thanks a ton
16:50EeemsMy pleasure. Just want to help things get fixed so I can try out the new experiments
16:50lorchardaaaand I've deleted it
16:50lorchardBut can repro the error so yay
16:52pauloiegasSVchuck lorchard I think Cosmin found the root cause
16:53pauloiegasSVEeems has the set browser content language to [en-ca] + [en]
16:53pauloiegasSVsince [en] is broken due to a bug that we have
16:54pauloiegasSVand probably we don't have [en-ca] localization
16:54pauloiegasSVthe browser cannot make any fallback and keeps loading the page
16:55lorchardYup, just got that bit in my profile and can reproduce
16:55pauloiegasSVEeems, can you please confirm the workaround by adding [en-us] on your profile please ?
16:55lorchardGoing to check the dev site to see if the PR I merged earlier fixes it
16:55chuckfzzzy: ^
16:55* Eeems grumbles about having to have en-US in his language list
16:55Eeemssure
16:56pauloiegasSVyou can do that from Firefox Options > Content > Language
16:56pauloiegasSVjust search for it and click the add button
16:56pauloiegasSVthen refresh the page
16:56pauloiegasSVwith Test Pilot
16:56lorchardHad a hunch locale might be involved, since we just did a huge l10n change, but I was trying other non-english locales that we had translations for :/
16:56Eeemsit works
16:56chuckEeems: no worries, just a diagnostic step. We'll fix it :)
16:56chuckCosminMCG: nice work!
16:56pauloiegasSVyep, nice work CosminMCG !!!
16:56lorchardAdded repro steps to the issue https://github.com/mozilla/testpilot/issues/2746#issuecomment-321864018
16:57pauloiegasSVholy crap, this bugged us for a few days :)
16:57CosminMCGthanks guys
16:58EeemsAnd this is why you always test strange language configurations when you have localization
16:58lorchardI should just start using en-CA since I'm like 10 minutes' drive from Windsor anyway
16:58Eeemshaha
16:59pauloiegasSVEeems: yeah, [en] was working okay until we landed a big push with changes for localization. Then it broke and we haven't checked the fallbacks
17:34_6a68miss u, test pilot ;_;
17:34_6a68gecko stuff is hard
17:35fzzzychuck lorchard i'm on pto but i think everyone is on the right track. try my pr
17:43lorchardYeah, that PR seems to fix the error I can repro
17:44lorchardFound another issue once that was fixed https://github.com/mozilla/testpilot/issues/2770
17:51j605In the recent update I saw that containers got moved and now I can't find a way to set a site open by default in a specific container
17:52chuckjkt: ^
20:35lorchardDecided I might try levelling up on flow types along the way to fixing a bug. Now my brain hurts
20:35lorchardFixed the bug at least, but I don't think I did the flow types right :/
21:20fzzzyhaha doh i did all that work to negotiate languages but then just passed navigator.languages??
21:22fzzzylorchard: you want to pass the union of all the actions like FooAction | BarAction.
21:22lorchardIt's more complicated than that... jotting things down in an issue
21:27fzzzySetLocalizationsAction | SetNegotiatedLanguagesAction should work
21:27lorchardIt passes linting but is not correct
21:28fzzzyodd
21:34lorchardfzzzy: There... I think I explained it https://github.com/mozilla/testpilot/issues/2772
21:35fzzzylorchard: aha, i think you are right. i forgot every action goes thru
21:35lorchardYeah, so I fell down a rabbit hole of looking into how other project handle this and I think I might give up for now and declare mfbt
21:36lorchardAnd also I didn't want to tie up what should have been a quick bugfix
21:36lorchardbut kind of expanded when I got the redux store involved :/
21:37fzzzyyeah :/ types, how do they work
21:38lorchardAnd I saw a few blog posts with flow type syntax that started looking like perl to "magically generate" types for actions & reducers and I just closed the window
21:39lorchardso yeah, tl;dr... let's use the negotiated locales to fix that bug. the rest looks like weird plumbing right now
21:39fzzzymaybe we can call flow a failed experiment
21:39fzzzythanks for fixing that bug!
21:40lorchardI might just need to put more time into wrapping my head around it, but it feels like a lot of paperwork :/
21:41fzzzyit really helped me to understand what state was actually in the store but i was never fully sure it was doing the right thing. and now it seems that it actually wasn't.
21:41fzzzymaybe typescript is better? dunno
21:42lorchardif we have time to switch to typescript, I'll worry it's because we don't have enough to do
21:42lorchardBut that could just be the accumulated grump talking :)
21:43lorchardI do worry sometimes that we've accumulated a lot of tooling & cruft for a mostly static site, but not entirely sure what's actionable about that
21:43lorchardbecause there have been good reasons for everything I can think of
21:46kumarI really like having Flow for redux actions/reducers because it can be really hard to figure out what the data structures look like without it.
21:46kumarAs a developer, I find that my first step is to fire up devtools to figure it out but then the next developer has to do it all over again.
21:47fzzzykumar, right, that's how it helped me. maybe you could comment about les' issue
21:47kumarWith Flow, you still have to do that in devtools but you can do it once and write down your findings in the type definitions.
21:47lorchardYeah, I think we need to rework the annotations to reflect how redux works
21:47fzzzykumar: how do you deal with this? https://github.com/mozilla/testpilot/issues/2772
21:47* kumar reads
21:48lorchardSeems like one of the key bits is annotating dispatch calls
21:49kumaryeah, I find that it's most important to annotate the dispatch calls
21:50kumarbecause that's where the bugs all happen: some code is dispatching the action incorrectly (it's very hard to add runtime checks for object mismatches in JS)
21:50lorchardannotating the reducer functions is kind of ignored, since our code never calls the reducers, just black box redux vendor code
21:50kumarand unit tests rarely catch that sort of thing
21:51lorchardSo, seems like dispatch and action creator functions are good to annotate
21:51lorchardwe can validate what goes in & comes out of action creators, and what goes into dispatch
21:52lorchardbut slice reducers are involved in a dynamic dispatch system driven by code we don't control
21:52kumarlorchard: I think the thing to do in a reducer is to declare the action as only the action you intend to operate on (there should only ever be one since all other actions are ignored)
21:52kumarlorchard: for example: https://github.com/mozilla/addons-frontend/blob/master/src/amo/reducers/viewContext.js#L24
21:52kumaraction here is declared only as ViewContextActionType
21:53lorchardYeah, but that reducer will see all actions
21:53kumarwhich is technically incorrect (we know) since it could be one of any action
21:53kumarbut with that approach, you are tricking Flow into giving you the coverage you want
21:53lorchardAnd there's nothing actually calling that which is subject to flow checking, is there?
21:53kumarwhat you want is to know if that reducer function is working with the payload *incorrectly*
21:54kumarright, so, Flow will not trace execution of the reducer
21:54kumarbut it will trace execution of all code *within* the reducer
21:54kumarwhich is the important bit
21:54kumarbecause in that example above, when we change action.payload.context to action.payload.somethingElse Flow will remind us to update it
21:55lorchardSecond problem I run into is here: https://github.com/mozilla/testpilot/pull/2771/commits/e1a86bbee74392c0b86c92e5f3ae26bfa1107fa1#diff-e94fde84a3b827ee9417e6e58c9f070bR34
21:55kumarthis is a really nice feature because in unit tests, one often deals with synthetic input (although I always try to dispatch real actions)
21:55lorchardi.e. we have the switch statement processing a generic action, passing the action to helper functions with specific action types annotated
21:56lorchardWas trying to figure out how to typecast a generic Action to e.g. SetLocalizationsAction
21:56lorchardwhich maybe the answer is, just don't organize it that way
21:57lorchardOr maybe our Action type can't be generic
21:57kumaractually, yeah, sorry I totally forgot that even in the same reducer you typically get multiple action types.
21:57kumarFlow cannot handle that at all
21:57lorchardYeah, that's the wall I'm headbanging :)
21:58lorchardBut, the missus is home, so I think it's MFBT and dinner time
21:58kumartheoretically you can use disjoint union types https://flow.org/en/docs/types/unions/#toc-disjoint-unions
21:58kumarbut it creates conflicts when two union'd types declare the same properties and all redux actions define a 'payload' property
22:01kumarI just remembered that my view context example only works because it only operates on one action type (this is not typical)
22:05b4handI think I am running into that load issue on ff57 on circleci though. https://287-87840961-gh.circle-artifacts.com/0/tmp/circle-artifacts.o6f3a73/integration-test-results/ui-test-dev.html
22:05b4handJavaScript error: https://example.com:8000/static/app/vendor.js, line 36433: TypeError: node is null
22:07kumarlorchard: fzzzy: the Flow docs have Redux examples now. They suggest using disjoint union types so maybe the conflicting properties problem (e.g. when multiple actions define 'payload') has been resolved. https://flow.org/en/docs/frameworks/redux/
22:08fzzzykumar: thanks for providing your insight!
22:08lorchardYeah, that's the disjoint union thing I mention in the issue... basically have to gather up the known set of all actions into one
22:09kumarwas it causing Flow errors to use disjoint unions?
22:09lorchardJust hadn't quite gotten to that point yet
22:10lorchardAnd doing it across the whole project was kind of ocean boiling, so I filed the issue to describe the problem
22:10kumarwell, for any given reducer, you only need to union together the actions that are expected for each case statement
22:10kumarinstead of all actions in the entire app
22:11lorchardBut every given reducer sees every action
22:11kumarin real life, yes
22:11lorchardI guess that's what I'm stuck on, and the dispatch() call should assert that whole set
22:11kumarbut you only care about validating the code that runs inside the reducer
22:12kumarso for Flow coverage purposes you can just declare the types of the actions that get passed to your specific reducer
22:12fzzzyok, so you are describing the way i did do it. cool.
22:12kumarbecause a reducer doesn't typically have a case statement for every action ever
22:12lorchardWhich is all the actions in the app :) because that's what gets passed
22:12kumaryes but I'm saying that a reducer ignores a lot of actions
22:13kumarso you can omit the ignored ones from the union type of your action parameter (as input to the reducer)
22:13lorchardYeah, I mean that just kind of feels like a no-op at that point
22:14lorchardsince the reducer is called from a dynamic dispatch in vendor code
22:14kumarfzzzy: ah, you got it working with union types? It didn't complain when two action types both define a 'payload' property? My version of Flow might have been outdated when I last tried it.
22:14lorchardAnyway, really gotta run, will poke at things maybe over the weekend
12 Aug 2017
No messages
   
Last message: 9 days and 21 hours ago