Skip to content
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

fix: use resolove to find imports in tests #715

Merged
merged 2 commits into from
Jan 26, 2021
Merged

fix: use resolove to find imports in tests #715

merged 2 commits into from
Jan 26, 2021

Conversation

safarmer
Copy link
Contributor

@safarmer safarmer commented Jan 22, 2021

testing changes referenced in code review for #714

This changes test imports to use resolve so that they can be run more easily from IDEs and tools
such as wallaby.js. Based on my testing, this looks like a safe change to make, and all tests pass
in VSCode, with node test, and in Wallaby.JS (running inside VSCode).

@safarmer safarmer requested a review from a team as a code owner January 22, 2021 04:51
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 22, 2021
@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #715 (5dba38f) into master (18ba840) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #715   +/-   ##
=======================================
  Coverage   87.20%   87.20%           
=======================================
  Files          55       55           
  Lines        6704     6705    +1     
  Branches      594      625   +31     
=======================================
+ Hits         5846     5847    +1     
  Misses        857      857           
  Partials        1        1           
Impacted Files Coverage Δ
src/github.ts 85.01% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18ba840...5dba38f. Read the comment docs.

wallaby.js Outdated
// See the License for the specific language governing permissions and
// limitations under the License.

module.exports = wallaby => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence about whether we should have configuration for a paid product in the top level of the library, what if instead you create a gist of this configuration, and we add a testing section to the README.md that could link to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I have removed the file from this PR and created https://gist.github.com/safarmer/7d112dd6e22bcf96286623b06e7191c3

@@ -495,7 +494,7 @@ describe('JavaYoshi', () => {
// check for default branch
.get('/repos/googleapis/java-trace')
// eslint-disable-next-line @typescript-eslint/no-var-requires
.reply(200, require('../../../test/fixtures/repo-get-2.json'))
.reply(200, require(resolve('./test/fixtures/repo-get-2.json')))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to land these changes.

This changes test imports to use `resolve` so that they can be run more easily from IDEs and tools
such as wallaby.js. Based on my testing, this looks like a safe change to make, and all tests pass
in VSCode, with node test, and in Wallaby.JS (running inside VSCode).
@bcoe
Copy link
Contributor

bcoe commented Jan 26, 2021

@safarmer thank you for the contribution, 👍. Do feel free to add a blurb in a testing section in the README for me to review.

@bcoe bcoe merged commit b0a6a18 into googleapis:master Jan 26, 2021
dazuma added a commit that referenced this pull request Feb 2, 2021
bcoe added a commit that referenced this pull request Feb 2, 2021
This reverts commit b0a6a18.

Co-authored-by: Daniel Azuma <dazuma@gmail.com>
@bcoe
Copy link
Contributor

bcoe commented Feb 2, 2021

@safarmer to land this again, I think we could actually potentially keep the resolve for tests, but anything actually in code needs to use the relative path.

I'm curious what other libraries might be doing that load package.json for version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants