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

chore(traits): added exhaustive test for trait function calls #2918

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

nickysn
Copy link
Contributor

@nickysn nickysn commented Sep 30, 2023

This test proves that trait function calls from inside the body of other trait functions are resolved and executed correctly. All combinations of:

default implementation
override with default
override with no default

are tested, for both the caller and callee. For now, this tests method calls only (functions with self as first parameter). Once function calls via Self::function_name() are implemented, the test can be extended to cover that as well.

This test proves that trait function calls from inside the body of other trait
functions are resolved and executed correctly.
@nickysn nickysn changed the title test(traits): added exhaustive test for trait function calls chore(traits): added exhaustive test for trait function calls Sep 30, 2023
@nickysn nickysn mentioned this pull request Sep 30, 2023
46 tasks
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

I don't think it's very practical to try testing every combination of something. Especially when adding a new dimension of something to test ends up increasing our test cases 4x. Instead of trying to test every single case, lets narrow it down to a select few that are more likely to cause issues for the compiler and aren't already tested by other integration tests.

@nickysn
Copy link
Contributor Author

nickysn commented Oct 3, 2023

I don't think it's very practical to try testing every combination of something. Especially when adding a new dimension of something to test ends up increasing our test cases 4x. Instead of trying to test every single case, lets narrow it down to a select few that are more likely to cause issues for the compiler and aren't already tested by other integration tests.

I know it's not always practical to test every combination of something, unless the number of test conditions is small, yielding a small number of combinations. In this case, we have 3 * 3 = 9 cases, that will be extended to 2 * 2 * 3 * 3 = 27 cases, once function calls via Self:: (a.k.a. static methods) are implemented. This is a very practical number of cases for an automated test and I'm not planning to increase the test conditions for this test any further, because it serves a very specific purpose - it tests whether function call resolution is done correctly with regards to choosing between trait default and trait impl ("override") implementation. This is also a feature that is specific to traits and is different, compared to e.g. struct impl function calls, where there's no such thing as default and override implementations (and combinations). We're not planning to adding any more dimensions to the test, that are unrelated to that (for example, once traits for non-struct types are implemented, we're not going to add every non-struct type as a new dimension to this test, but we'll definitely add a new test that just implements a trait for each type - that's because implementing traits for non-struct types is something that is completely independent from trait function call resolution).

@nickysn nickysn requested a review from jfecher October 3, 2023 08:44
@jfecher jfecher added this pull request to the merge queue Oct 3, 2023
Merged via the queue into noir-lang:master with commit bc90ba0 Oct 3, 2023
21 of 22 checks passed
TomAFrench added a commit that referenced this pull request Oct 3, 2023
* master:
  feat: Add ACIR serializer C++ codegen (#2961)
  chore(traits): added exhaustive test for trait function calls (#2918)
Sakapoi pushed a commit to Sakapoi/noir_fork that referenced this pull request Oct 19, 2023
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.

None yet

2 participants