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.

Sign up for GitHub

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

Jump to bottom

Fix prog/main_test.go #2567

Merged
merged 6 commits into from
Jun 8, 2017
Merged

Fix prog/main_test.go #2567

merged 6 commits into from
Jun 8, 2017

Conversation

rndstr
Copy link
Contributor

@rndstr rndstr commented Jun 6, 2017

Is excluded from tests and does not compile.

@rndstr
Copy link
Contributor Author

rndstr commented Jun 6, 2017

Not sure what the consensus is on testing private interfaces; if "okay" then I can re-add and fix it

@2opremio
Copy link
Contributor

2opremio commented Jun 6, 2017

Good catch, sigh. I think we should re-add it.

@rndstr
Copy link
Contributor Author

rndstr commented Jun 6, 2017

@2opremio ok if I use github.com/stretchr/testify/assert? It's already in vendor/

@2opremio
Copy link
Contributor

2opremio commented Jun 6, 2017

Sure

@rndstr rndstr changed the title Remove dead prog/main_test.go test Fix prog/main_test.go Jun 6, 2017
@rndstr rndstr force-pushed the remove-dead-tests branch from 2d994d6 to 5e55748 Compare June 7, 2017 09:55
@rndstr
Copy link
Contributor Author

rndstr commented Jun 7, 2017 •
edited
Loading

Executing any backend tests in prog/ requires the "scope ui asset providers" (staticui & externalui) to be generated. These in turn have a dependency on the client app which then needs to be fully built. For this to work we would need to move the scope-ui-backend docker image to a node based image (or additionally install node && npm). Which is in my opinion not worth the trouble till we actually do have asset-delivery tests.

Edit: So instead of doing that I just skip the npm build in case we are running from within the scope-ui-backend image.

Edit2: ...which is rubbish since the scope-ui-backend image is also in use for buliding other tasks than prog/scope

rndstr
rndstr commented Jun 7, 2017
View reviewed changes
Makefile Outdated
@@ -179,10 +179,12 @@ tmp/weave-scope.tgz: $(shell find client/app -type f) $(SCOPE_UI_BUILD_UPTODATE)
else

client/build/index.html:
cd client && npm run build
test "true" = "$(SCOPE_SKIP_UI_ASSETS)" && { mkdir -p client/build; } || \

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio
Copy link
Contributor

2opremio commented Jun 7, 2017 •
edited
Loading

Which is in my opinion not worth the trouble till we actually do have asset-delivery tests.

I agree. I didn't expect this to open a can of worms.

@rndstr
Copy link
Contributor Author

rndstr commented Jun 8, 2017

@2opremio I was wrong with Edit2, should work as intended & ready for review :)

@rndstr rndstr merged commit b78745f into master Jun 8, 2017
@rndstr rndstr deleted the remove-dead-tests branch June 8, 2017 13:41
bboreham
bboreham reviewed Jun 19, 2017
View reviewed changes
tools/test
@@ -74,7 +74,7 @@ fail=0

if [ -z "$TESTDIRS" ]; then
# NB: Relies on paths being prefixed with './'.
TESTDIRS=($(git ls-files -- '*_test.go' | grep -vE '^(vendor|prog|experimental)/' | xargs -n1 dirname | sort -u | sed -e 's|^|./|'))
TESTDIRS=($(git ls-files -- '*_test.go' | grep -vE '^(vendor|experimental)/' | xargs -n1 dirname | sort -u | sed -e 's|^|./|'))

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bboreham bboreham bboreham left review comments

@2opremio 2opremio 2opremio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rndstr @2opremio @bboreham