Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

chore: add missing dev dependency #635

Merged

Conversation

DominicKramer
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2019
kjin
kjin previously approved these changes Feb 13, 2019
@DominicKramer DominicKramer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 13, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 13, 2019
@kjin kjin dismissed their stale review February 13, 2019 22:42

Use teeny-request?

@ofrobots
Copy link
Contributor

ofrobots commented Feb 13, 2019

It might be better to review and land my deflake PR first. Sorry, wrong repo.

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM.

IMO, for a devDependency, request is fine.

@DominicKramer DominicKramer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@DominicKramer
Copy link
Contributor Author

I'm not able to get the system tests to fail locally using the exact same node version on either Linux or OS X. So I'm still investigating the failure.

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1368b62). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #635   +/-   ##
=========================================
  Coverage          ?   92.91%           
=========================================
  Files             ?       38           
  Lines             ?     3544           
  Branches          ?      105           
=========================================
  Hits              ?     3293           
  Misses            ?      228           
  Partials          ?       23
Impacted Files Coverage Δ
test/auth-request.ts 83.33% <100%> (ø)

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 1368b62...6175dc4. Read the comment docs.

@ofrobots
Copy link
Contributor

@DominicKramer Looking at the failure, it seems like we are getting a nock failure. We seem to be trying to contact the metadata service to look up an instance property.

The e2e test already has a nock for the projectId, so the only remaining metadata request woul be this one:

const clusterName = await Debuglet.getClusterNameFromMetadata();

The only reason we would reach this line is if onGCP is true. When you're running things locally, this would not be true. I am not sure what has changed recently for the system tests to exercise the paths differently however.

Anyhow, there are probably two ways you can fix this.

  1. Add a nock for the cluster name.
  2. Modify the test to provide a service context bypassing this code.

@ofrobots
Copy link
Contributor

I have a tentative fix for 2 in this commit you can cherry pick: 7367af3

@DominicKramer DominicKramer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@DominicKramer DominicKramer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2019
@DominicKramer DominicKramer merged commit 8a9ea83 into googleapis:master Feb 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants