Skip to content

Removes arbitrary limits from eqValues & eqArray#303

Open
twig2let wants to merge 1 commit into
rokucommunity:masterfrom
twig2let:remove-arbitrary-comparison-limits
Open

Removes arbitrary limits from eqValues & eqArray#303
twig2let wants to merge 1 commit into
rokucommunity:masterfrom
twig2let:remove-arbitrary-comparison-limits

Conversation

@twig2let
Copy link
Copy Markdown
Contributor

@twig2let twig2let commented Nov 1, 2024

We're seeing REACHED MAX ITERATIONS DOING COMPARISON warnings in our unit tests which can potentially result in false positives since true is always returned once the call count is reached.

I was in the process of making this max iteration count configurable but what would be a sensible default? Should we enforce a max call count outside the BrightScript engine?

As it stands, these warnings are logged to console and are easily missed, especially when the test suite is of a significant size.

These changes are a proposal to remove the hardcoded call count in favour of letting BrightScript enforce stack limits.

@TwitchBronBron
Copy link
Copy Markdown
Member

My guess is that this prevents the entire test application from crashing, because if brightscript enforces their limits, the current test crashing could bring down the app. If this is the justification for this feature existing, then I'm in favor of keeping the check.

Perhaps rather than removing the checks, we should make the function return false when we encounter this issue instead? That way a test will fail rather than bringing down the app or lying by saying the items were equal.

@georgejecook can you chime in here with why you added this functionality in the first place?

@twig2let
Copy link
Copy Markdown
Contributor Author

twig2let commented Nov 1, 2024

Yeah, sounds reasonable regarding not wanting to crash the tests but Rooibos is enforcing limitations which don't exist in BrightScript i.e the implementation code might be perfectly fine but Rooibos would fail it because of an arbitrary limit, assuming we return false by default.

If the behaviour is desired by default, perhaps we could introduce a new config flag to ignore it?

@georgejecook
Copy link
Copy Markdown
Collaborator

We do this because in situations where you have a circular graph, it crashes. this was a crude workaround I added years ago, that worked fine for the use case I had (in maestro, classes have a reference to top, and when debugging, top dumps the class fields, which include top - though many other opportunities exist to get this to happen).

I think it's definitely worthy of revisiting, and apoligize for the headaches this must have caused you and others.. I wish I used ticktick back then ;)

@TwitchBronBron
Copy link
Copy Markdown
Member

@chrisdp and I have some ideas for properly detecting circular references when building comparing objects. That might make this entire MAX ITERATIONS irrelevant. Will update this ticket if we get a chance to implement it..

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