Skip to content

Conversation

@brackendawson
Copy link
Collaborator

@brackendawson brackendawson commented Oct 17, 2022

Summary

In YAMLEq(), yaml documents in the input after the first document were silently not considered in the comparison. Make it compare all the docs.

I checked JSONEq() and it always fails if you try to pass it JSON Lines documents, even if they are the same.

Changes

Motivation

Unlike with passing JSON Lines inputs to JSONEq(), which always fails, passing multiple YAML documents with differences after the first document into YAMLEq() silently passes. 😱

Related issues

Closes #1281

Copy link
Collaborator

@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase on master.

@dolmen dolmen added pkg-assert Change related to package testify/assert must-rebase bug labels Jul 25, 2023
@dolmen dolmen changed the title subsequent YAML documents were ignored YAMLEq: handle multi documents Jul 25, 2023
@brackendawson brackendawson added this to the v1.8.6 milestone Feb 24, 2024
@brackendawson brackendawson requested a review from dolmen February 24, 2024 19:49
@brackendawson brackendawson modified the milestones: v1.9.1, v1.10 Mar 5, 2024
@dolmen dolmen added the YAML About YAML and dependency label Mar 19, 2024
@brackendawson brackendawson modified the milestones: v1.10, v1.11 Dec 13, 2024
In YAMLEq(), yaml documents in the input after the first document were
silently not considered in the comparison. Make it compare all the docs.

I checked JSONEq() and it always fails if you try to pass it JSON Lines
documents, even if they are the same.
@dolmen
Copy link
Collaborator

dolmen commented May 28, 2025

Related: #1755 (also changes YAMLEq)

@dolmen dolmen changed the title YAMLEq: handle multi documents assert.YAMLEq: handle multi documents May 28, 2025
@dolmen
Copy link
Collaborator

dolmen commented Jun 1, 2025

This patch has been broken by #1579 which made the YAML library pluggable.

Updating the patch requires changing the interface for pluggable YAML lib as yaml.Unmarshal doesn't support multidocs: this patch uses a yaml.Decoder, so rebasing is not trvial.

@brackendawson
Copy link
Collaborator Author

Because #1579 exposed the internal implementation of YAMLEq on testify's API, it is not possible to solve this issue without either partially implementing a YAML parser in testify itself or changing testify's API in one of two ways:

  • Change assert.yaml.Unmarshal to a decoder. This would be a breaking change so no.
  • Add another YAML assertion (like YAMLEqAll) that uses decode. I dislike this because we shouldn't have added YAMLEq in the first place.

I'm closing this PR because it's no longer tenable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug must-rebase pkg-assert Change related to package testify/assert YAML About YAML and dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

YAMLEq: multi documents are not checked

2 participants