Skip to content

Conversation

@philals
Copy link
Contributor

@philals philals commented Jul 18, 2017

Hey @jordanwalsh23

Nice meeting you the other day. Good work with this.

I think the express stuff was in the dependencies due to the sample app. I also moved nyc to dev-dependency.

I ran npm i and yarn install which updated the lock files for both package managers.

All the tests were green except one. I don't think it was because of my changes, but have not confirmed yet.

image

@jordanwalsh23
Copy link
Contributor

Thanks @philals.

Yea that test is a bit painful.

It's a counting the number of contacts that have been modified in the last 30 seconds. If all goes according to plan, the test prior modifies a contact, so when this runs it should only return one result.

The problem is that sometimes it takes longer to run a few tests, and you end up modifying the contact 30+ seconds prior. Which means this test returns 0 results, which means it fails.

I should harden it, but normally what I do is just bump up the 30 seconds to 60 seconds if I need the tests to pass.

I'll review the changes and merge it in tomorrow.

@philals
Copy link
Contributor Author

philals commented Jul 18, 2017

Cheers @jordanwalsh23

No rush on the PR.

Is that to test to test xero-nodes modifiedsince handling? Or to test that it's updating a contact?

Just thinking about it....

@jordanwalsh23
Copy link
Contributor

jordanwalsh23 commented Jul 18, 2017 via email

@philals
Copy link
Contributor Author

philals commented Jul 19, 2017

Cool. Nice.

Can't think of a better way to test it right now.

Sorry I thought we were using NPM
@philals
Copy link
Contributor Author

philals commented Jul 19, 2017

Added a commit that removes the package-lock.json

Will only use yarn from now

@philals
Copy link
Contributor Author

philals commented Jul 19, 2017

#53 is my preferred on to go first.

I'll re-update the lock file once that's in.

@jordanwalsh23
Copy link
Contributor

#53 merged. Can you please push with the latest lock file and I'll review. Thx!

@philals
Copy link
Contributor Author

philals commented Jul 20, 2017

Cheers @jordanwalsh23 All green

@jordanwalsh23 jordanwalsh23 merged commit 0a974e7 into XeroAPI:master Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants