tests(lantern): add lantern regression test scripts #5435

patrickhulce · 2018-06-05T19:23:41Z

Adds lantern regression testing against ~100 URLs to travis (only when lantern files are affected).

A few things this PR highlighted:

Our scripts are getting sophisticated and probably a candidate for some refactoring into libs and such
We duplicate a decent bit of logic in our scripts, perhaps moving more to js and aforementioned refactoring would help.
Names are hard and suggestions welcome. It felt weird to call the lantern expectations file we checkin a "golden" file since it's not really golden, it's where we are right now and WPT results are the golden expectations we're trying to reach. I sorta went with computed vs. expected but these aren't great and not 100% consistent.

patrickhulce · 2018-06-12T23:52:06Z

I think this is the last thing standing in my way before #5362 #5482 #5483 etc 😃

paulirish · 2018-06-13T03:09:06Z

I sorta went with computed vs. expected but these aren't great and not 100% consistent.

what do each of these groups represent? i'm not sure between WPT data, 'golden' lantern data, and freshly computed lantern data.. what we're dealing with here.

patrickhulce · 2018-06-13T21:02:03Z

what do each of these groups represent? i'm not sure between WPT data, 'golden' lantern data, and freshly computed lantern data.. what we're dealing with here.

we have a few main things

lantern-data/lantern-expectations.json this contains URLs and the WPT metric values, this is our target against which accuracy is judged
lantern-data/lantern-computed.json this contains the URLs and computed Lantern values for the current build
core/test/fixtures/lantern-expectations.json this contains the URLs and computed Lantern values for master which is diff'd similar to our golden LHR. this is the one that is most in need of a name change, but I'm not sure what to call it :)

paulirish · 2018-06-13T22:01:50Z

perfect. this helps enormously. thanks!

some name ideas below:

lantern-data/lantern-expectations.json this contains URLs and the WPT metric values, this is our target against which accuracy is judged

wpt-baseline-data

lantern-data/lantern-computed.json this contains the URLs and computed Lantern values for the current build

lantern-fresh-data

core/test/fixtures/lantern-expectations.json this contains the URLs and computed Lantern values for master which is diff'd similar to our golden LHR. this is the one that is most in need of a name change, but I'm not sure what to call it :)

lantern-master-data

I'm also fine with switching the order of baseline/fresh/master and wpt/lantern.

brendankenny

LGTM

Is there going to be a failure condition?

brendankenny · 2018-06-14T00:34:11Z

lighthouse-core/scripts/test-lantern.sh

+# Testing lantern can be expensive, we'll only run the tests if we touched files that affect the simulations.
+CHANGED_FILES=""
+if [[ "$CI" ]]; then
+  CHANGED_FILES=$(git --no-pager diff --name-only $TRAVIS_COMMIT_RANGE)


$TRAVIS_COMMIT_RANGE

cooool

patrickhulce · 2018-06-14T19:29:54Z

Is there going to be a failure condition?

Hm, yeah seems useful to match golden LHR pattern here for now, I'll exit with 1 if there was any difference 👍

patrickhulce · 2018-06-14T19:38:43Z

@paulirish I oversimplified the lantern-expectations one a bit which I only realized once wpt-baseline-data sounded wrong to me.

It contains the list of all the URLs we should compare, the WPT metric values, and the paths to the devtools log and trace of the unthrottled runs. Just WPT feels a bit weird in that scenario since it's an index of the unthrottled data around it. If I were going verbose I'd probably say something like
lantern-3g-golden-expectations-index.json
lantern-3g-HEAD-computed-values.json
lantern-3g-master-computed-values.json

paulirish

sorry! lots of questions.
we probably have to work through this in voice. :)

evaluate-results.js - this is evaluating the correlation of wpt data with our HEAD lantern data, right? print-correlations.js?
diff-expectations.js - this has nothing to do with wpt data but is logging out any difference between master...HEAD of lantern metrics, right? do we need this script to pass? or is this a sanity check so we are cognizant of any changes we make? assert-master-lantern-values-unchanged.js?

lantern-3g-master-computed-values.json

this name lgtm, though i'd be fine with dropping 3g.

lantern-3g-golden-expectations-index.json

this one not lgtm.

first, i'm a little wary of using 'expectations' especially since we use it in smokehouse and it feels like the semantics here are different.

~~i still feel like calling this wpt-baseline is fair. you said:~~

Just WPT feels a bit weird in that scenario since it's an index of the unthrottled data around it.

~~but i dont understand. if the data has no throttling applied then why would it be ...-3g-... ? i dont get why it'd be called lantern either.~~

~~and perhaps a rather macro question: why are we correlating simulated 3g against unthrottled metrics? i totally was expecting that we're comparing all this to wpt-3g metrics.~~

site-index-plus-golden-expectations
site-index-plus-golden-expectations-plus-HEAD-computed

paulirish · 2018-06-14T19:55:46Z

lighthouse-core/scripts/lantern/diff-expectations.js

+    exitCode = 1;
+  }
+} finally {
+  fs.unlinkSync(TMP_COMPUTED);


~~how do these files compare to lantern-data/lantern-{computed|expected}.json?~~

can we move -data/lantern-computed to .tmp?

maybe these should have -for-diff in the filename? (if i'm understanding why they're separate)

paulirish · 2018-06-14T19:59:30Z

lighthouse-core/scripts/lantern/download-traces.sh

 tar -xzf lantern-traces.tar.gz
-mv lantern-traces-subset lantern-data
+mv lantern-traces-subset/ lantern-data/


can these live in something like lantern-data/unthrottledAssets/ ?

so that lantern-data/lantern-expected.json are not siblings to them?

paulirish · 2018-06-14T20:06:48Z

lighthouse-core/scripts/lantern/update-expectations.js

+const EXPECTATIONS_PATH = path.resolve(process.cwd(), INPUT_PATH);
+const EXPECTATIONS_DIR = path.dirname(EXPECTATIONS_PATH);
+const COMPUTED_PATH = path.join(EXPECTATIONS_DIR, 'lantern-computed.json');
+const RUN_ALL_SCRIPT_PATH = path.join(__dirname, 'run-all-expectations.js');


seems like this isn't running expectations but "collecting computed values". wdyt?

paulirish · 2018-06-14T20:28:01Z

lighthouse-core/scripts/lantern/download-traces.sh

 DIRNAME="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 LH_ROOT_PATH="$DIRNAME/../../.."
 cd $LH_ROOT_PATH

+if [[ -f lantern-data/lantern-expectations.json ]] && ! [[ "$FORCE" ]]; then
+  echo "Lantern data already detected, done."


within the lantern-data/lantern-expectations.json file....

can these metrics be within a wpt3g object? and then put the trace/log props inside a unthrottledAssets obj?

paulirish · 2018-06-14T20:37:05Z

lighthouse-core/scripts/lantern/evaluate-results.js

 const path = require('path');

 const GOOD_ABSOLUTE_THRESHOLD = 0.2;
 const OK_ABSOLUTE_THRESHOLD = 0.5;

 const GOOD_RANK_THRESHOLD = 0.1;

-if (!process.argv[2]) throw new Error('Usage $0 <computed summary file>');
+const COMPUTATIONS_INPUT_ARG = process.argv[2] || './lantern-data/lantern-computed.json';


one of these things doesn't look like the others. COMPUTATIONS?

patrickhulce · 2018-06-15T19:05:40Z

@paulirish how you feelin' about this now :)

paulirish · 2018-06-15T23:27:11Z

merge when you're ready

patrickhulce · 2018-06-15T23:29:24Z

let's do it! 🎉

patrickhulce added 2 commits June 5, 2018 12:08

tests(lantern): add golden lantern test scripts

ddb018c

add test-lantern script

94eab43

patrickhulce changed the title ~~tests(lantern): add golden lantern test scripts~~ tests(lantern): add lantern regression test scripts Jun 5, 2018

patrickhulce added 5 commits June 6, 2018 10:14

set -e download

2c4d23d

temporarily disable other tests

e51a64c

add debug ls

cb8bd11

modified donwload

86c4c22

uncomment travis

466162b

brendankenny approved these changes Jun 14, 2018

View reviewed changes

exit with 1 if there are changes

94be4c5

paulirish reviewed Jun 14, 2018

View reviewed changes

mega feedback

56355bc

paulirish approved these changes Jun 15, 2018

View reviewed changes

patrickhulce merged commit b263f83 into master Jun 15, 2018

patrickhulce deleted the lantern_each_commit branch June 15, 2018 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(lantern): add lantern regression test scripts #5435

tests(lantern): add lantern regression test scripts #5435

patrickhulce commented Jun 5, 2018

patrickhulce commented Jun 12, 2018

paulirish commented Jun 13, 2018

patrickhulce commented Jun 13, 2018

paulirish commented Jun 13, 2018

brendankenny left a comment

brendankenny Jun 14, 2018

patrickhulce commented Jun 14, 2018

patrickhulce commented Jun 14, 2018

paulirish left a comment •

edited

Loading

paulirish Jun 14, 2018 •

edited

Loading

paulirish Jun 14, 2018

paulirish Jun 14, 2018 •

edited

Loading

paulirish Jun 14, 2018

paulirish Jun 14, 2018 •

edited

Loading

paulirish Jun 14, 2018

patrickhulce commented Jun 15, 2018

paulirish commented Jun 15, 2018

patrickhulce commented Jun 15, 2018

tests(lantern): add lantern regression test scripts #5435

tests(lantern): add lantern regression test scripts #5435

Conversation

patrickhulce commented Jun 5, 2018

patrickhulce commented Jun 12, 2018

paulirish commented Jun 13, 2018

patrickhulce commented Jun 13, 2018

paulirish commented Jun 13, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jun 14, 2018

Choose a reason for hiding this comment

patrickhulce commented Jun 14, 2018

patrickhulce commented Jun 14, 2018

paulirish left a comment • edited Loading

Choose a reason for hiding this comment

paulirish Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish Jun 14, 2018

Choose a reason for hiding this comment

paulirish Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish Jun 14, 2018

Choose a reason for hiding this comment

paulirish Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish Jun 14, 2018

Choose a reason for hiding this comment

patrickhulce commented Jun 15, 2018

paulirish commented Jun 15, 2018

patrickhulce commented Jun 15, 2018

paulirish left a comment •

edited

Loading

paulirish Jun 14, 2018 •

edited

Loading

paulirish Jun 14, 2018 •

edited

Loading

paulirish Jun 14, 2018 •

edited

Loading