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

Point to healthcheck inside of the dist folder always #72

Closed
wants to merge 1 commit into from

Conversation

Robdel12
Copy link
Contributor

@Robdel12 Robdel12 commented Apr 2, 2019

What is this?

This removes the healthcheck dance that uses require and path, which aren't available in the browser. It wasn't quite transpiling properly (#69), so if this doesn't actually work well, we should look at making changes to how this SDK is distributed.

It looks like cy.exec takes care of any cross platform issues. All tests pass -- I'm curious what this might be missing? Seems like cy.exec also resolves the path properly. Before merge I'd like to cut a beta and have some folks try it out that had the healthcheck issues in #61. Just to be 100% sure we're not regressing (since we haven't been able to replicate)

Open question

Is this healthcheck really needed? I'm curious what it's meant to do? Is it to provide helpful logging for when a snapshot fails? How does it fail if the server isn't available? cy.request() doesn't fail the cypress test build and we can .catch any errors from it and provide nice logs from there I believe.

It might be that this pain can just go away with that. Curious to hear thoughts on this.

It looks like `cy.exec` takes care of any cross platform issues. All
tests pass -- I'm curious what this might be missing? `require` isn't
available in the browser and neither is `path`. Seems like `cy.exec`
also resolves the path properly.
@Robdel12
Copy link
Contributor Author

Robdel12 commented Apr 3, 2019

Closing this. We’re going to implement a health check CLI command in agent to avoid all of these path issues

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.

1 participant