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

Implement a possibility to run TestCafe over https (close #1985) #2540

Merged
merged 4 commits into from
Jun 25, 2018

Conversation

miherlosev
Copy link
Collaborator

@miherlosev miherlosev commented Jun 20, 2018

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 3032990 have failed. See details:

@@ -107,6 +107,7 @@ export default class CLIArgumentParser {
.option('--ports <port1,port2>', 'specify custom port numbers')
.option('--hostname <name>', 'specify the hostname')
.option('--proxy <host>', 'specify the host of the proxy server')
.option('--ssl', 'run the TestCafe proxy server over the HTTPS protocol')
Copy link
Contributor

Choose a reason for hiding this comment

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

            .option('--ssl', 'run TestCafe proxy server over the HTTPS protocol')

Maybe we should rename the flag to --https considering the description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. Need to change description.
This flag contains ssl options that will passed for https server creation.

Also, this option has same name (ssl) for the widely known http-server module.
Changed description: specify ssl options for starting TestCafe using https protocol

Copy link
Contributor

Choose a reason for hiding this comment

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

specify ssl options

So, you can actually specify some options after this flag? Which options?

Copy link
Collaborator Author

@miherlosev miherlosev Jun 21, 2018

Choose a reason for hiding this comment

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

Yes, you ca specify a lot of options (all options from https server options and parent classes: http server iptions, tls server options).

See a simple example in #1985 (comment)

Copy link
Contributor

@VasilyStrelyaev VasilyStrelyaev Jun 21, 2018

Choose a reason for hiding this comment

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

Cool!
As far as I understand, options are not required, so we can write

            .option('--ssl <options>', 'enable access to TestCafe via the HTTPS protocol and specify SSL options')

Copy link
Contributor

@VasilyStrelyaev VasilyStrelyaev Jun 21, 2018

Choose a reason for hiding this comment

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

            .option('--ssl <options>', 'specify SSL options for starting TestCafe proxy server over the HTTPS protocol');

Copy link
Contributor

Choose a reason for hiding this comment

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

            .option('--ssl <options>', 'specify SSL options to run TestCafe proxy server over the HTTPS protocol');

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 175d4da have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 175d4da have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 175d4da have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 175d4da have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 175d4da have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 831fa31 have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 831fa31 have passed. See details:

@miherlosev
Copy link
Collaborator Author

FPR @VasilyStrelyaev @AndreyBelym

value = convertToBestFitType(value);

if (FILE_OPTION_NAMES.includes(key) && value.length < MAX_PATH_LENGTH)
value = fs.readFileSync(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

On Linux MAX_PATH_LENGTH is 4096 (4kB). My SSH private key is about 1.5kB. If I pass my certificate, I will get an error from fs.readFileSync. We should check if the file exists, and if it don't, also assume that value is a buffer.

import fs from 'fs';
import os from 'os';

const MAX_PATH_LENGTH = os.type() === 'Windows_NT' ? 260 : Infinity;
Copy link
Contributor

Choose a reason for hiding this comment

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

MAX_PATH_LENGTH === Infinity means you can't pass a certificate content for properties that accept both file paths and file contents.

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit e800581 have passed. See details:

@miherlosev
Copy link
Collaborator Author

FPR @VasilyStrelyaev @AndreyBelym

@miherlosev
Copy link
Collaborator Author

ping @VasilyStrelyaev

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 37e9f98 have failed. See details:

@miherlosev
Copy link
Collaborator Author

@testcafe-build-bot retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 37e9f98 have passed. See details:

@miherlosev miherlosev merged commit 4be7652 into DevExpress:master Jun 25, 2018
@miherlosev miherlosev deleted the https branch June 26, 2018 08:07
kirovboris pushed a commit to kirovboris/testcafe-phoenix that referenced this pull request Dec 18, 2019
…1985) (DevExpress#2540)

* implement a possibility to run TestCafe over https

* update text

* update option description

* fix review issues
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.

4 participants