Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/e2e/data/invalid-tomls/bad-syntax.toml
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
11 changes: 11 additions & 0 deletions packages/e2e/data/invalid-tomls/invalid-webhook.toml
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"]
9 changes: 9 additions & 0 deletions packages/e2e/data/invalid-tomls/wrong-type.toml
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"
5 changes: 5 additions & 0 deletions packages/e2e/data/valid-app/package.json
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
}
46 changes: 46 additions & 0 deletions packages/e2e/data/valid-app/shopify.app.toml
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"
34 changes: 34 additions & 0 deletions packages/e2e/setup/toml-app.ts
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})
},
})
46 changes: 46 additions & 0 deletions packages/e2e/tests/toml-config-invalid.spec.ts
Copy link
Contributor

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_urls
  • invalid-api-version.toml - invalid webhooks.api_version
  • invalid-direct-api-mode.toml - access.admin.direct_api_mode = "invalid"
  • wrong-array-type.toml - auth.redirect_urls as string instead of array

These would provide more comprehensive coverage of the validation rules implemented in the codebase.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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})
}
})
}
})
39 changes: 39 additions & 0 deletions packages/e2e/tests/toml-config.spec.ts
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()
}
})
})
Loading