Skip to content

tests: add change-tracking tests for embedded attributes#2166

Merged
zachdaniel merged 1 commit intoash-project:mainfrom
rellen:tests/add-change-tracking-tests-for-attributes-that-are-embedded-attributes
Jun 26, 2025
Merged

tests: add change-tracking tests for embedded attributes#2166
zachdaniel merged 1 commit intoash-project:mainfrom
rellen:tests/add-change-tracking-tests-for-attributes-that-are-embedded-attributes

Conversation

@rellen
Copy link
Contributor

@rellen rellen commented Jun 26, 2025

These tests check that change-tracking in changesets works as expected for changes to attributes that are embedded resources.

We are tracking down a strange bug somewhere in Ash or AshPaperTrail, and these tests imply that the issue is not in Ash, but thought it would be worthwhile to add them to the test suite.

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

These tests check that change-tracking in changesets works as expected
for changes to attributes that are embedded resources.
Comment on lines +682 to +684
{_result, notifications} = Ash.update!(changeset, return_notifications?: true)

assert List.first(notifications).changeset.context[:changed?] == false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this an acceptable way to get access to the changeset after the change has been executed?

Another way could be to add an after_action hook to send a message with the changeset back to the test process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...yeah, I think the latter would be the preferred way in this scenario. I don't want to necessarily lean on the notification.changeset particulars

Copy link
Contributor

Choose a reason for hiding this comment

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

actually this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might also be good to assert on the creation or non-creation of versions as that is the primary use case we're concerned with here. However, that can be done in addition to what's already been done, so I'll merge this.

@zachdaniel zachdaniel merged commit b5d4e96 into ash-project:main Jun 26, 2025
43 checks passed
@zachdaniel
Copy link
Contributor

🚀 Thank you for your contribution! 🚀

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.

2 participants