-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: remove protoc roundtrip check from update:sample-json #10557
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
details-renderer-test.js
console errors currently emitted duringyarn 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)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question on variable naming
|
||
let sampleResultsRoundtripStr; | ||
try { | ||
sampleResultsRoundtripStr = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exterkamp is this something you'd like to fix?
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔔 🔔 🔔 🎉 🎉
this is a great victory @connorjclark |
See: #10549 (comment)