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

Removes arbitrary limits from eqValues & eqArray #303

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

twig2let
Copy link
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
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
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
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 ;)

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