-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add more tests for API generation script #581
Conversation
versionWithoutPatch: "0.0", | ||
hasSeparateReleaseNotes: true, | ||
title: "My Quantum Software Project", | ||
} as Pkg; |
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 an important trick to make tests more readable, rather than us having to give dummy values for attributes that aren't relevant. We should audit existing tests to use this when possible.
const descriptionSuffix = pkg.hasSeparateReleaseNotes | ||
? `in ${pkg.title}${versionStr}` | ||
: `to ${pkg.title}`; |
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 behavior change for Qiskit, but same for Runtime and Provider.
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.
Could we use to
for both cases?
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 English reads poorly to say "Changes made to Qiskit 0.45" because it sounds like you're only changing Qiskit 0.45, when in reality you are changing Qiskit 0.45+. It's not wrong, but makes less sense.
(Prepositions are the worst part of languages for me lol)
|
||
describe("dedupeIds", () => { |
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 test, I only removed the unnecessary describe()
.
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.
Nice work! Thank you for adding all these tests, it will help a lot ensuring that everything works as intended and understanding some parts of the code better. I added a new test in addFrontMatter.test.ts to verify the order of the conditionals if we have python_api_name defined and also hardcoded_frontmatter. I think this will be useful for #66.
I left a couple of comments, but I approve the PR for when you think it's ready to merge, all worked well on my end!
const descriptionSuffix = pkg.hasSeparateReleaseNotes | ||
? `in ${pkg.title}${versionStr}` | ||
: `to ${pkg.title}`; |
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.
Could we use to
for both cases?
Co-authored-by: Arnau Casau <[email protected]>
Part of #223. One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves. This PR also makes some light refactoring like renaming things to be more clear. --------- Co-authored-by: Arnau Casau <[email protected]> Co-authored-by: Arnau Casau <[email protected]>
Part of Qiskit#223. One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves. This PR also makes some light refactoring like renaming things to be more clear. --------- Co-authored-by: Arnau Casau <[email protected]> Co-authored-by: Arnau Casau <[email protected]>
Part of #223.
One way to improve the maintainability of our API generation is to test more. It gives us confidence we aren't breaking things and it makes it more obvious how the function behaves.
This PR also makes some light refactoring like renaming things to be more clear.