Skip to content
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

Synchronizing shared expression tests with frontend #1040

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

olemartinorg
Copy link
Contributor

Description

I compared the folders with those in app-frontend-react, and updating tests to match.

A few notes:

  • I've added shared tests for argv. Positional arguments didn't seem to support null, so I changed the signatures to do that.
  • Most of the changes to tests were caused by me adding altinnRowId to rows (back when that was implemented - frontend has no functionality to automatically add them)
  • Frontend can no longer gracefully handle duplicate component IDs, so I removed a few of those tests. I'm still keeping the ones in invalid, but those have been removed in frontend as well (it's just very cumbersome to test that there, because it happens long before the expression engine is even ready).

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green (not on my machine, but we'll see what github says!)

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Ole Martin Handeland added 4 commits January 16, 2025 12:54
Copy link
Member

@ivarne ivarne left a comment

Choose a reason for hiding this comment

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

Good!

@ivarne
Copy link
Member

ivarne commented Jan 16, 2025

We have a folder named test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/up-for-evaluation where you can put shared tests that don't (yet) work in backend

@olemartinorg
Copy link
Contributor Author

We have a folder named test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/up-for-evaluation where you can put shared tests that don't (yet) work in backend

Yes, I saw that. Was about to copy those folder over there when you wrote this comment, but after thinking more about it I came to the conclusion that I don't really like that pattern:

  1. When comparing folders (I use Meld for that), these all appear as different. There's no good way to tell such a tool that 'these folders are actually the same as these other folders you see over there', so it just adds to the list of things I ingore when diffing.
  2. They're never really used in the backend tests anyway, so I guess they're just there to remind developers passing by that there are some functions that still need to be implemented. I think the docs page should be the place for that, or issues on github of course.
  3. Since the diffing marks these tests as entirely different anyway, I never really looked at those tests in up-for-evaluation when syncing this time. I think that can be misleading, because if someone picks up one of those folder, decides to implement that function and use the shared tests to validate - they may already be outdated and cause you to implement the wrong thing.

So, is there a good way to include them along with all the other test files, but ignore them in the tests instead? That way the diff between frontend and backend gets to be very clean, and you get updated tests whenever we sync across repos (and in the future if we convert to a monorepo, these tests will be ready for that as well).

@ivarne
Copy link
Member

ivarne commented Jan 16, 2025

So, is there a good way to include them along with all the other test files, but ignore them in the tests instead? That way the diff between frontend and backend gets to be very clean, and you get updated tests whenever we sync across repos (and in the future if we convert to a monorepo, these tests will be ready for that as well).

Yes, there is [Theory(Skip = "Not implemented yet")]. They will show up in Skipped tests statistics, but that is probably what we want. I totally see your arguments against the up-for-evaluation folder, but in that case we should delete it instead of having it outdated.

@ivarne ivarne merged commit 2965048 into main Jan 16, 2025
13 of 14 checks passed
@ivarne ivarne deleted the chore/sync-shared-expr branch January 16, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🧪 Test
Development

Successfully merging this pull request may close these issues.

2 participants