Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

Make HTTPS tests fail if they are loaded over HTTP #207

Closed
wants to merge 1 commit into from
Closed

Make HTTPS tests fail if they are loaded over HTTP #207

wants to merge 1 commit into from

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Aug 26, 2016

The goal of this PR is to make the test harness fail HTTPS-only tests when they are loaded over HTTP with a "Test page must be loaded over HTTPS" message.

The assumption is that such tests are defined in files that end with .https.html.


This change is Reviewable

Test files that end with ".https.html" trigger a check on the current protocol.
If the current protocol is not "https:", all tests in this test file fail with
a "Test page must be loaded over HTTPS" message.
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 26, 2016

r? @jgraham

@sideshowbarker
Copy link
Contributor

This PR is still waiting on review. @jgraham?

@jgraham
Copy link
Member

jgraham commented Nov 2, 2016

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


testharness.js, line 495 at r1 (raw file):

        var pos = 0;
        if (location && location.pathname) {
            pos = location.pathname.indexOf(".https.html");

The extension doesn't have to be .html. You possibly want to split on . and check for https in a slice(1, length - 1) i.e. excluding the extension and anything before the first ..


testharness.js, line 514 at r1 (raw file):

        properties = properties ? properties : {};
        var test_obj = new Test(test_name, properties);
        test_obj.step(enforce_https, test_obj, test_obj);

I don't think it makes sense to run this for every test. We could just run the check and make it throw so that the harness status comes back error.


Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Nov 2, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 3, 2016

@tidoust are you able to address the review comments?

@tidoust
Copy link
Member Author

tidoust commented Nov 3, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


testharness.js, line 514 at r1 (raw file):

Previously, jgraham wrote…

I don't think it makes sense to run this for every test. We could just run the check and make it throw so that the harness status comes back error.

Actually, I do not know how to make the harness come back with an error status no matter what.

For instance, if I throw near the end of the initialization phase, say right before/after the call to on_tests_ready(), the error appears in the console but won't be reported. If I throw in the Test constructor, the error event handler will get triggered, but it will fail to report anything in the case where file_is_test is set (because it expects a Test instance in that case). If I throw after the call to the constructor in the test and async_test functions, then this mostly works as expected except that the harness will only report after timeout and will loop forever if explicit_timeout was set to true for some reason.

Could you clarify where you'd like the check to run and throw?


Comments from Reviewable

@annevk
Copy link
Member

annevk commented Mar 30, 2017

@tidoust it seems this code still doesn't split on "." and then checks if one of them is "https".

@tidoust
Copy link
Member Author

tidoust commented Mar 30, 2017

@annevk I seem to have lost the initial branch and repo with which I created that PR, so I created a new PR #255 with the suggested update. I'm closing this one as a result.

Note that the new PR still does not address @jgraham's comment that he would like to "just run the check and make it throw so that the harness status comes back error". As said above, I do not know how to do that properly with the current code.

@tidoust tidoust closed this Mar 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants