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

Use mocha for cert/redirect tests and add content-type tests #25

Merged
merged 10 commits into from
Oct 30, 2017

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 29, 2017

Rename the directory to test to match the mocha convention, even though
the package.json is inside the directory and the test path (*.js) is
given explicitly there.

Rename the directory to test to match the mocha convention, even though
the package.json is inside the directory and the test path (*.js) is
given explicitly there.
@foolip
Copy link
Member Author

foolip commented Oct 29, 2017

This can be seen working in https://travis-ci.org/whatwg/misc-server/builds/294578969, which worked before I dropped aa66797 from the branch.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Not sure I see the need for the rename given that this isn't a subdirectory of the main package directory.

You can add e.g. --slow 1000 to get rid of the red times in the CI output.

.travis.yml Outdated
before_install:
- cd test
install:
- npm install
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary; it's implied by node_js

.travis.yml Outdated
script:
- ./run-tests.sh
- npm test
Copy link
Member

Choose a reason for hiding this comment

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

Also unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Both dropped.

test/certs.js Outdated
try {
const cert = await request;
for (const domain of DOMAINS) {
it(domain, async function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I like using specify here instead of it; they are equivalent but since you're not doing it("should do X", ...) it reads better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good, I didn't like it or understand what it meant, but didn't know there was an option.

['https://whatwg.org/demos/repeat-02/results.xml', 'application/xml'],
];

const WITHOUT_EXTENSION_TESTS = [
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be comprehensive? Seems a pain to keep this in sync every time we add a file....

Copy link
Member Author

Choose a reason for hiding this comment

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

It's all extensionless files I could find currently, but no, I don't think we should keep updating this, I think we should stop adding such files, or at least only use it for HTML for pretty URLs.

['https://whatwg.org/specs/web-forms/2004-12-10-call-for-comments/sample-multiple-editable-ui', 'image/png'],
['https://whatwg.org/specs/web-forms/2004-12-10-call-for-comments/xforms-implementation-diagram', 'image/png'],
// much more of the same...
/*
Copy link
Member

Choose a reason for hiding this comment

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

Why keep this commented-out block?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make the tests run a bit faster by skipping some bits. And easier testing of everything when doing the nginx setup. But between deleting and including, I think I'll just run all of the tests, and maybe we can make this smaller later using http://nginx.org/en/docs/http/ngx_http_core_module.html#try_files.

@foolip
Copy link
Member Author

foolip commented Oct 30, 2017

OK, all fixed I think, added some more things I found in #7. https://travis-ci.org/whatwg/misc-server/builds/294777142 passed before I reverted the .travis.yml change.

@foolip foolip merged commit 6243a12 into master Oct 30, 2017
@foolip foolip deleted the mocha-tests branch October 30, 2017 16:27
foolip added a commit that referenced this pull request Oct 31, 2017
The redirects are translated from the .htaccess deleted in
whatwg/url#351 and tested in
#25.
@foolip foolip mentioned this pull request Oct 31, 2017
foolip added a commit that referenced this pull request Oct 31, 2017
The redirects are translated from the .htaccess deleted in
whatwg/url#351 and tested in
#25.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants