-
Notifications
You must be signed in to change notification settings - Fork 233
Add TOML config regression tests #6940
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # Invalid TOML: malformed syntax (missing closing quote) | ||
| client_id = "__E2E_CLIENT_ID__" | ||
| name = "Bad Syntax App | ||
| application_url = "https://example.com" | ||
| embedded = true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # Invalid TOML: bad webhook config (missing required uri) | ||
| client_id = "__E2E_CLIENT_ID__" | ||
| name = "Invalid Webhook App" | ||
| application_url = "https://example.com" | ||
| embedded = true | ||
|
|
||
| [webhooks] | ||
| api_version = "2025-01" | ||
|
|
||
| [[webhooks.subscriptions]] | ||
| topics = ["products/create"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # Invalid TOML: wrong types for known fields | ||
| client_id = "__E2E_CLIENT_ID__" | ||
| name = "Wrong Type App" | ||
| application_url = "https://example.com" | ||
| embedded = "not-a-boolean" | ||
|
|
||
| [access_scopes] | ||
| scopes = 12345 | ||
| use_legacy_install_flow = "nope" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "name": "e2e-toml-regression-test", | ||
| "version": "1.0.0", | ||
| "private": true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Comprehensive shopify.app.toml for E2E regression testing | ||
| # client_id is injected at runtime by the toml-app fixture | ||
| client_id = "__E2E_CLIENT_ID__" | ||
| name = "E2E TOML Regression Test" | ||
| application_url = "https://example.com" | ||
| embedded = true | ||
|
|
||
| [access_scopes] | ||
| scopes = "read_products,write_products,read_orders" | ||
| use_legacy_install_flow = false | ||
|
|
||
| [access.admin] | ||
| direct_api_mode = "online" | ||
| embedded_app_direct_api_access = true | ||
|
|
||
| [auth] | ||
| redirect_urls = [ | ||
| "https://example.com/auth/callback", | ||
| "https://example.com/auth/shopify/callback", | ||
| ] | ||
|
|
||
| [webhooks] | ||
| api_version = "2025-01" | ||
|
|
||
| [[webhooks.subscriptions]] | ||
| topics = ["products/create"] | ||
| uri = "https://example.com/webhooks/products/create" | ||
|
|
||
| [[webhooks.subscriptions]] | ||
| topics = ["orders/create"] | ||
| uri = "https://example.com/webhooks/orders/create" | ||
|
|
||
| [app_proxy] | ||
| url = "https://example.com/proxy" | ||
| subpath = "e2e-test" | ||
| prefix = "apps" | ||
|
|
||
| [pos] | ||
| embedded = false | ||
|
|
||
| [build] | ||
| automatically_update_urls_on_dev = true | ||
| include_config_on_deploy = true | ||
|
|
||
| [app_preferences] | ||
| url = "https://example.com/preferences" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* eslint-disable no-restricted-imports */ | ||
| import {authFixture} from './auth.js' | ||
| import * as path from 'path' | ||
| import * as fs from 'fs' | ||
| import {fileURLToPath} from 'url' | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url) | ||
| const __dirname = path.dirname(__filename) | ||
|
|
||
| const FIXTURE_DIR = path.join(__dirname, '../data/valid-app') | ||
|
|
||
| /** | ||
| * Test fixture that copies the full-toml fixture into a temp directory, | ||
| * injects the real client_id, and exposes the path to tests. | ||
| */ | ||
| export const tomlAppFixture = authFixture.extend<{tomlAppDir: string}>({ | ||
| tomlAppDir: async ({env, authLogin: _authLogin}, use) => { | ||
| const appDir = fs.mkdtempSync(path.join(env.tempDir, 'toml-app-')) | ||
|
|
||
| // Copy fixture files | ||
| for (const file of fs.readdirSync(FIXTURE_DIR)) { | ||
| fs.copyFileSync(path.join(FIXTURE_DIR, file), path.join(appDir, file)) | ||
| } | ||
|
|
||
| // Inject real client_id | ||
| const tomlPath = path.join(appDir, 'shopify.app.toml') | ||
| const toml = fs.readFileSync(tomlPath, 'utf8') | ||
| fs.writeFileSync(tomlPath, toml.replace('__E2E_CLIENT_ID__', env.clientId)) | ||
|
|
||
| await use(appDir) | ||
|
|
||
| fs.rmSync(appDir, {recursive: true, force: true}) | ||
| }, | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* eslint-disable no-console */ | ||
| /* eslint-disable no-restricted-imports */ | ||
| import {authFixture as test} from '../setup/auth.js' | ||
| import {requireEnv} from '../setup/env.js' | ||
| import {expect} from '@playwright/test' | ||
| import * as path from 'path' | ||
| import * as fs from 'fs' | ||
| import {fileURLToPath} from 'url' | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)) | ||
| const INVALID_TOMLS_DIR = path.join(__dirname, '../data/invalid-tomls') | ||
|
|
||
| const invalidTomls = fs.readdirSync(INVALID_TOMLS_DIR).filter((file) => file.endsWith('.toml')) | ||
|
|
||
| test.describe('TOML config invalid', () => { | ||
| for (const tomlFile of invalidTomls) { | ||
| const label = tomlFile.replace('.toml', '') | ||
|
|
||
| test(`deploy rejects invalid toml: ${label}`, async ({cli, env}) => { | ||
| requireEnv(env, 'clientId') | ||
|
|
||
| // Set up temp dir with invalid toml + minimal package.json | ||
| const appDir = fs.mkdtempSync(path.join(env.tempDir, `invalid-toml-${label}-`)) | ||
| try { | ||
| const toml = fs | ||
| .readFileSync(path.join(INVALID_TOMLS_DIR, tomlFile), 'utf8') | ||
| .replace('__E2E_CLIENT_ID__', env.clientId) | ||
| fs.writeFileSync(path.join(appDir, 'shopify.app.toml'), toml) | ||
| fs.writeFileSync( | ||
| path.join(appDir, 'package.json'), | ||
| JSON.stringify({name: `invalid-${label}`, version: '1.0.0', private: true}), | ||
| ) | ||
|
|
||
| const result = await cli.exec(['app', 'deploy', '--path', appDir, '--force'], { | ||
| timeout: 2 * 60 * 1000, | ||
| }) | ||
| const output = result.stdout + result.stderr | ||
| console.log(`[${label}] exit code: ${result.exitCode}\n[${label}] output:\n${output}`) | ||
| expect(result.exitCode, `expected deploy to fail for ${label}, but it succeeded:\n${output}`).not.toBe(0) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Improvement: The PR description states the CLI should 'reject invalid configurations with clear errors', but the test only validates that the command fails (non-zero exit code) without verifying that error messages are actually clear and helpful. The test captures output for diagnostics but doesn't assert anything about error message content or quality. The CLI could return unhelpful errors like 'Unknown error' and the test would still pass. Suggestion: Add assertions to verify that error messages are present and meaningful: expect(result.exitCode, `expected deploy to fail for ${label}, but it succeeded:\n${output}`).not.toBe(0)
expect(output.toLowerCase()).toMatch(/error|invalid|failed/)
expect(output).toContain('shopify.app.toml')Or create specific assertions for each invalid case (e.g., verify 'bad-syntax' mentions syntax errors, 'invalid-webhook' mentions missing uri). |
||
| expect(output.toLowerCase(), `expected error output for ${label}:\n${output}`).toMatch(/error|invalid|failed/) | ||
| } finally { | ||
| fs.rmSync(appDir, {recursive: true, force: true}) | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /* eslint-disable no-console */ | ||
| import {tomlAppFixture as test} from '../setup/toml-app.js' | ||
| import {requireEnv} from '../setup/env.js' | ||
| import {expect} from '@playwright/test' | ||
|
|
||
| test.describe('TOML config regression', () => { | ||
| test('deploy succeeds with fully populated toml', async ({cli, env, tomlAppDir}) => { | ||
| requireEnv(env, 'clientId') | ||
|
|
||
| const result = await cli.exec(['app', 'deploy', '--path', tomlAppDir, '--force'], { | ||
| timeout: 5 * 60 * 1000, | ||
| }) | ||
| const output = result.stdout + result.stderr | ||
| expect(result.exitCode, `deploy failed:\n${output}`).toBe(0) | ||
| }) | ||
|
|
||
| test('dev starts with fully populated toml', async ({cli, env, tomlAppDir}) => { | ||
| test.setTimeout(6 * 60 * 1000) | ||
| requireEnv(env, 'clientId', 'storeFqdn') | ||
|
|
||
| const proc = await cli.spawn(['app', 'dev', '--path', tomlAppDir], { | ||
| env: {CI: ''}, | ||
| }) | ||
|
|
||
| try { | ||
| await proc.waitForOutput('Ready, watching for changes in your app', 3 * 60 * 1000) | ||
|
|
||
| proc.sendKey('q') | ||
| const exitCode = await proc.waitForExit(30_000) | ||
| expect(exitCode, `dev exited with non-zero code. Output:\n${proc.getOutput()}`).toBe(0) | ||
| } catch (error) { | ||
| const captured = proc.getOutput() | ||
| console.error(`[toml-config dev] Captured PTY output:\n${captured}`) | ||
| throw error | ||
| } finally { | ||
| proc.kill() | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Improvement: The test suite includes 5 invalid TOML cases but is missing several categories of validation issues that exist in the actual codebase. Additional valuable test cases could include: URL validation failures (invalid URLs in app_proxy.url, auth.redirect_urls), webhook API version validation, array vs string type mismatches, invalid enum values (e.g., access.admin.direct_api_mode accepts only 'online'/'offline'), and field length validations. More comprehensive coverage would provide stronger regression protection.
Suggestion: Consider adding test files for additional validation scenarios:
invalid-url.toml- malformed URL in application_url or redirect_urlsinvalid-api-version.toml- invalid webhooks.api_versioninvalid-direct-api-mode.toml- access.admin.direct_api_mode = "invalid"wrong-array-type.toml- auth.redirect_urls as string instead of arrayThese would provide more comprehensive coverage of the validation rules implemented in the codebase.