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 browser command with non-default root path. #75

Merged
merged 2 commits into from
Mar 10, 2016
Merged

Conversation

mstemm
Copy link

@mstemm mstemm commented Mar 8, 2016

Make the browser command easier to use when run against a
juttle-engine/juttle-service with a root path other than /. Do this by
fetching the server's root directory and comparing that to the path
provided in the --path argument. If the root directory isn't a prefix
of the --path argument, throw an error. Otherwise, take just the part
not covered by the --root argument and use that as the ?path= cgi
param in the url.

In juttle-engine-client, also fully resolve the path immediately
instead of several times in the function for each command.

Also update juttle-engine to rely on juttle-service's built-in
defaults with juttle-config.js and command-line overrides. This
simplifies the initialization steps a bit.

These changes caused the test coverage numbers to dip slightly below
the thresholds, so provide some additional options when creating the
engine to cover one additional branch, which got the coverage numbers
up again.

This PR depends on changes in juttle-service (juttle/juttle-service#67). I can wait to merge this until we're ready to push a new juttle-service, or do an early juttle-service release (bumping the minor number), and update package.json for juttle-engine to depend on it.

@demmer @go-oleg @VladVega

return relative_to_root;
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

do we want this to be possible for juttle-service to not have a root_path? If this is the case then juttle-service/path should always throw an error. I've put up a pr that would not let you initialize a juttle-service without a root_directory juttle/juttle-service#68

Copy link
Author

Choose a reason for hiding this comment

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

I think with the changes in juttle/juttle-service#67 (and prior to that as well), it's not possible to have a juttle-service without a root path, as there's a default root.

But I'm all for explicitly failing on the server side as well.

Wrt the client, this is more just being cautious, so I think it's ok to still have the check.

Mark Stemm added 2 commits March 10, 2016 14:11
Make the browser command easier to use when run against a
juttle-engine/juttle-service with a root path other than /. Do this by
fetching the server's root directory and comparing that to the path
provided in the --path argument. If the root directory isn't a prefix
of the --path argument, throw an error. Otherwise, take just the part
not covered by the --root argument and use that as the ?path= cgi
param in the url.

In juttle-engine-client, also fully resolve the path immediately
instead of several times in the function for each command.

Also update juttle-engine to rely on juttle-service's built-in
defaults with juttle-config.js and command-line overrides. This
simplifies the initialization steps a bit.

These changes caused the test coverage numbers to dip slightly below
the thresholds, so provide some additional options when creating the
engine to cover one additional branch, which got the coverage numbers
up again.
Verify that these changes work properly with the changes on
juttle-service by using the 0.4.0 rc release.
@demmer
Copy link
Contributor

demmer commented Mar 10, 2016

+1

mstemm pushed a commit that referenced this pull request Mar 10, 2016
Fix browser command with non-default root path.
@mstemm mstemm merged commit 2919230 into master Mar 10, 2016
@mstemm mstemm deleted the change-root-to-cwd branch March 10, 2016 22:58
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.

None yet

3 participants