Skip to content

Conversation

@bobzhang
Copy link
Contributor

No description provided.

@peter-jerry-ye-code-review
Copy link

peter-jerry-ye-code-review bot commented Oct 16, 2025

Duplicated test code across multiple files

Category
Maintainability
Code Snippet
Lines 106-128 in arbitrary_test.mbt and lines 24-41 in utils.mbt both define similar Test0 traits and test cases
Recommendation
Consolidate the trait definition and tests into a single location, or clearly differentiate the test scenarios if they serve different purposes
Reasoning
Code duplication makes maintenance harder and can lead to inconsistent behavior if one copy is updated but not the other

Inconsistent trait visibility and parameter syntax

Category
Correctness
Code Snippet
arbitrary_test.mbt uses 'trait Test0' (public) with mixed parameter syntax 'x~ : Int = 3, y? = 2, z?', while utils.mbt uses 'priv trait Test0' with consistent 'x? : Int = 3, y? = 2'
Recommendation
Use consistent parameter syntax across all trait definitions. Choose either '?' or '' for optional parameters and apply consistently
Reasoning
The comment in arbitrary_test.mbt suggests confusion about the syntax ('weird both x
and y? work here??'), indicating the need for standardization

Incomplete test coverage for edge cases

Category
Maintainability
Code Snippet
The test in arbitrary_test.mbt includes parameter 'z?' but utils.mbt doesn't, and the 'z?' parameter is not tested in all scenarios
Recommendation
Either remove the unused 'z?' parameter or add comprehensive test cases that cover all parameter combinations including when 'z' is None
Reasoning
Incomplete test coverage can hide bugs and the unused parameter suggests unclear requirements or incomplete implementation

@coveralls
Copy link
Collaborator

coveralls commented Oct 16, 2025

Pull Request Test Coverage Report for Build 1525

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 89.214%

Totals Coverage Status
Change from base Build 1523: 0.001%
Covered Lines: 9388
Relevant Lines: 10523

💛 - Coveralls

inspect((10).sum(), content="15")
// weird warnings appear here
// Calling local `impl` with `.` or `Type::meth(..)` is deprecated, use `Trait::meth(..)` instead.
// this seems to be inconsistent with the fact that `Type::meth` will call `Trait::meth`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

upstream T::new_f (x.new_f) conflict with (x.new_f)

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.

3 participants