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

verification of content-type and status code for http file server exercise #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jpdamon
Copy link

@jpdamon jpdamon commented Jan 4, 2016

Clarified that setting the content-type and status code are now required. Added verification to exercise.js
Fixes #348

@martinheidegger
Copy link
Contributor

Continuing conversation here:

Yes, the tests obviously didn't cover this particular use-case before so it would be good to have a test-case covering this particular problem. The test cases are quite simple: Just add a valid_0x.js file to the test/http_file_server/ folder that - without your fix - would have failed. You then can run the tests again using npm test.

Also it would be good if the PR would contain the locales in all languages - even if they are in english - since there is no fallback yet.

@jpdamon
Copy link
Author

jpdamon commented Jan 5, 2016

Makes sense, will do.

The issue I'm having though is that learnyounode verify still passes even when an error is emitted. How do I actually trigger a failure? This is my first contribution to this project so I apologize if this is answered somewhere else.

@martinheidegger
Copy link
Contributor

😱 I only read the first part of your last comment. .emit('fail', ...) should work but currently doesn't - this is sort of okay - I need to fix this at a different levels of learnyounode and workshopper-adventure at more places than this. I plan to fix this all in one bunch (since it requires some restructuring) but here is the workshopper bug: workshopper/workshopper-adventure#81

@shanmukhateja
Copy link
Contributor

This PR seems to be okay to be merged. Although its 2 years old, we could have somebody take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP FILE SERVER solution contains unnecessary/unexplained line
3 participants