tests: remove protoc roundtrip check from update:sample-json #10557
Conversation
brendankenny
changed the title
There was a problem hiding this comment.
ok, a couple of things, but I'm very excited about this change.
I am a little nervous that this is going to get tripped up and stop running at some point and we won't notice since the tests will just get skipped, but I guess that's what PSI canaries and integration tests are for? :)
| // eslint-disable-next-line no-console | ||
| console.warn('Skipping test - you need to run yarn test-proto first.'); | ||
| } | ||
| const itIf = sampleResultsRoundtripStr ? it : it.skip; |
There was a problem hiding this comment.
this is a clever solution, though I'm a little worried it's too clever and we'll lose track that it's happening :)
I can only think of worse alternatives, though. Any testPathIgnorePatterns or clever globbing is much uglier and non-local to the actual code that depends on the round trip file existing.
What do you think about just a more explicit name, like const itIfProtoExists or something?
| fs.readFileSync(__dirname + '/../../../results/sample_v2.json', 'utf-8'); | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Skipping test - you need to run yarn test-proto first.'); |
There was a problem hiding this comment.
The details-renderer-test.js console errors currently emitted during yarn unit-core are already distracting (visually triggering as if there was a failure), can we avoid adding more of them? Is a comment in the file sufficient? (since you'd presumably come here to diagnose the problem)
There was a problem hiding this comment.
The
details-renderer-test.jsconsole errors currently emitted duringyarn unit-coreare already distracting (visually triggering as if there was a failure), can we avoid adding more of them? Is a comment in the file sufficient? (since you'd presumably come here to diagnose the problem)
oh, do console.warn messages get skipped in jest output? I don't see these in the travis log
There was a problem hiding this comment.
Nah, but I was reading the wrong file here, perhaps that's why you didn't see it.
| // eslint-disable-next-line no-console | ||
| console.warn('Skipping test - you need to run yarn test-proto first.'); | ||
| } | ||
| const describeIf = roundTripJson ? describe : describe.skip; |
There was a problem hiding this comment.
same question on variable naming
|
|
||
| let sampleResultsRoundtripStr; | ||
| try { | ||
| sampleResultsRoundtripStr = |
There was a problem hiding this comment.
it's weird that only one test in this file is using sampleResultsRoundtripStr even though the PSI renderer will only be getting lhrs round-tripped through a proto...
There was a problem hiding this comment.
@exterkamp is this something you'd like to fix?
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
|
this is a great victory @connorjclark |
See: #10549 (comment)