Skip to content

SEP-1936: Fix the signed-URL test assertion that flakes across an itsdangerous timestamp boundary - #1459

Merged
yyyyyyyan merged 11 commits into
mainfrom
SEP-1936
Sep 3, 2026
Merged

SEP-1936: Fix the signed-URL test assertion that flakes across an itsdangerous timestamp boundary#1459
yyyyyyyan merged 11 commits into
mainfrom
SEP-1936

Conversation

@peter-o-addo

@peter-o-addo peter-o-addo commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Compare decoded itsdangerous payloads instead of raw signed URLs in snippet execution-meta equality checks so tests no longer flake across a second boundary.

  • tests/app/sep/snippets/test_script_source.py: decode snippet_source before comparing metas in the legacy-path and extra-args alias tests

Tested

Skip test

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally (make test)
  • Pre-commit hooks pass (make run-pre-commit)
  • Database migrations generated if models changed (make makemigrations)
  • User-facing changes documented (README, inline help, UI text)
  • Configuration changes documented with examples
  • Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)

@peter-o-addo
peter-o-addo marked this pull request as ready for review September 3, 2026 08:00
Copilot AI balanced review requested due to automatic review settings September 3, 2026 08:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The normalization unintentionally removes URL base-path coverage from the parity assertions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents signed-URL test flakes by comparing decoded token payloads instead of timestamp-sensitive URLs.

Changes:

  • Adds signed-token decoding helpers.
  • Updates two execution-metadata equality assertions.
File summaries
File Description
tests/app/sep/snippets/test_script_source.py Normalizes signed URLs in metadata comparisons.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/app/sep/snippets/test_script_source.py Outdated
Comment thread tests/app/sep/snippets/test_script_source.py Outdated
Comment thread tests/app/sep/snippets/test_script_source.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new helpers need complete docstrings and should avoid duplicating the artifact route literal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

tests/app/sep/snippets/test_script_source.py:107

  • This new helper's docstring does not document its meta parameter or returned dump, leaving the helper contract incomplete. Expand it with the corresponding rST fields.
def _meta_dump_decoded(meta: SnippetExecutionMeta) -> dict[str, object]:
    """Return ``meta.model_dump()`` with ``snippet_source`` timestamp-normalized."""
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread tests/app/sep/snippets/test_script_source.py
Comment thread tests/app/sep/snippets/test_script_source.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused test-only change removes timestamp sensitivity while preserving meaningful URL and payload validation.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@peter-o-addo peter-o-addo added the qa not required Merge without a QA sign-off: substitutes for 'qa passed' in label-gate. Does not skip any test job. label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Coverage report

This PR does not seem to contain any modification to coverable code.

@yyyyyyyan
yyyyyyyan enabled auto-merge (squash) September 3, 2026 16:55
@yyyyyyyan
yyyyyyyan merged commit 78764df into main Sep 3, 2026
18 checks passed
@yyyyyyyan
yyyyyyyan deleted the SEP-1936 branch September 3, 2026 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python qa not required Merge without a QA sign-off: substitutes for 'qa passed' in label-gate. Does not skip any test job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants