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

Supports Cypress v10 or later #156

Merged
merged 6 commits into from Jun 5, 2023

Conversation

S-H-GAMELINKS
Copy link
Contributor

Hi.

I tried this gem when introducing E2E testing to my Mastodon server, but I had to edit some configuration files.
Currently, generated configuration file is pre-Cypress v10 (as far as I can tell from checking the example).

So, I wrote a patch to generate configuration files for Cypress v10 or later (including the latest v12.13.0).

I am not sure what policy should be used for the following part only. I would be happy to discuss this in this pull request.

  • if user already has Cypress configuration file, the existing code overwrites cypress.json. Should we keep this behavior?

@searls
Copy link
Member

searls commented Jun 5, 2023

v10 is old enough now that it probably makes sense to release this, assuming everything works

@searls
Copy link
Member

searls commented Jun 5, 2023

CI is failing -- did the example tests pass for you locally?

@S-H-GAMELINKS
Copy link
Contributor Author

S-H-GAMELINKS commented Jun 5, 2023

CI is failing -- did the example tests pass for you locally?

Yes. my local environment passed all test.

Comment on lines 23 to 42
def call(dir = Dir.pwd)
config_path = File.join(dir, "cypress.json")
json = JSON.pretty_generate(determine_new_config(config_path))
File.write(config_path, json)
config_path = File.join(dir, "cypress.config.js")
config_content = determine_new_config(config_path)
File.write(config_path, config_content)
puts "Cypress config (re)initialized in #{config_path}"
end

private

def determine_new_config(config_path)
if File.exist?(config_path)
merge_existing_with_defaults(config_path)
merge_existing_with_defaults(config_path)
else
DEFAULT_CONFIG
end
end

def merge_existing_with_defaults(json_path)
JSON.parse(File.read(json_path)).merge(DEFAULT_CONFIG).sort.to_h
def merge_existing_with_defaults(config_path)
File.read(config_path)
end
Copy link
Member

Choose a reason for hiding this comment

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

Since merging JS files isn't feasible for this gem to try to do here, I think we should avoid writing to the file ssytem entirely if cypress.config.js is present (as opposed to reading the file only to overwrite, as is done here) and instead warn that the file already exists and so we will not be re-initializing it.

Make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments!
Warn for users when running cypress:init and Cypress config already exist.

ref: c376925

Comment on lines 3 to 21
DEFAULT_CONFIG = <<~CYPRESS_CONFIG
const { defineConfig } = require('cypress')

module.exports = defineConfig({
// setupNodeEvents can be defined in either
// the e2e or component configuration
e2e: {
setupNodeEvents(on, config) {
on('before:browser:launch', (browser = {}, launchOptions) => {
/* ... */
})
},
},
screenshotsFolder: "tmp/cypress_screenshots",
videosFolder: "tmp/cypress_videos",
trashAssetsBeforeRuns: false
})

CYPRESS_CONFIG
Copy link
Member

Choose a reason for hiding this comment

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

Recommend reformatting:

Suggested change
DEFAULT_CONFIG = <<~CYPRESS_CONFIG
const { defineConfig } = require('cypress')
module.exports = defineConfig({
// setupNodeEvents can be defined in either
// the e2e or component configuration
e2e: {
setupNodeEvents(on, config) {
on('before:browser:launch', (browser = {}, launchOptions) => {
/* ... */
})
},
},
screenshotsFolder: "tmp/cypress_screenshots",
videosFolder: "tmp/cypress_videos",
trashAssetsBeforeRuns: false
})
CYPRESS_CONFIG
DEFAULT_CONFIG = <<~CYPRESS_CONFIG
const { defineConfig } = require('cypress')
module.exports = defineConfig({
// setupNodeEvents can be defined in either
// the e2e or component configuration
e2e: {
setupNodeEvents(on, config) {
on('before:browser:launch', (browser = {}, launchOptions) => {
/* ... */
})
},
},
screenshotsFolder: "tmp/cypress_screenshots",
videosFolder: "tmp/cypress_videos",
trashAssetsBeforeRuns: false
})
CYPRESS_CONFIG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These Cypress setting content formatted in this commit.
ref: ea4319e

@S-H-GAMELINKS S-H-GAMELINKS requested a review from searls June 5, 2023 15:05
@searls searls merged commit 1d015a5 into testdouble:main Jun 5, 2023
2 checks passed
@S-H-GAMELINKS S-H-GAMELINKS deleted the support/cypress-v10or-later branch June 5, 2023 15:20
@searls
Copy link
Member

searls commented Jun 5, 2023

Thank you! Released as 0.6.0

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.

None yet

2 participants