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

Add nested loop tests for else clause in loops #2630

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

advikkabra
Copy link
Collaborator

Continuation of #2555 .
Nested loops were not added completely in tests.

@kmr-srbh
Copy link
Contributor

kmr-srbh commented Mar 27, 2024

Hello @advikkabra ! Thanks for this. I think we can add a reference test for this to check the control flow output. @Thirumalai-Shaktivel, @Shaikh-Ubaid what do think about it?

@Shaikh-Ubaid
Copy link
Collaborator

I think we can add a reference test for this to check the control flow output

I think integration tests are fine for this.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft March 28, 2024 07:01
@Shaikh-Ubaid
Copy link
Collaborator

Please mark as "Ready for review" when ready.

@advikkabra advikkabra marked this pull request as ready for review March 28, 2024 15:12
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

Thanks for this! I think we need to fix the following:

  • There seems to be a bug in the for-else implementation.
  • Test cases need to be robust by adding asserts on variables values.

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft March 29, 2024 02:12
@advikkabra advikkabra marked this pull request as ready for review March 30, 2024 18:14
Comment on lines 25 to 26
print(10)
assert j == 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also print the values before the asserts. That helps during debugging.

Comment on lines 43 to 48
print("outer: " + str(i))
for j in range(10, 20):
print(" inner: " + str(j))
break
else:
print("no break in outer loop")
return
assert False
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for return here. Just increment some variable (inside the loops, before the breaks, etc.). You can use different variables for different loops here.
At the end of the function print the final values for the variables and add asserts for them. I think this would make it a much more robust test. You can follow this approach for other tests as well. Thanks!

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft April 3, 2024 07:04
@advikkabra advikkabra marked this pull request as ready for review April 3, 2024 19:45
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thanks!

@Shaikh-Ubaid Shaikh-Ubaid enabled auto-merge (squash) April 4, 2024 03:26
@Shaikh-Ubaid Shaikh-Ubaid merged commit 760c898 into lcompilers:main Apr 4, 2024
13 checks passed
assem2002 pushed a commit to assem2002/lpython that referenced this pull request Apr 28, 2024
* Add nested loop tests for else clause in loops

* Add nested else test

* Fix bug in for else implementation

* Update tests to use assert

* Add print statements in tests
assem2002 pushed a commit to assem2002/lpython that referenced this pull request Apr 28, 2024
* Add nested loop tests for else clause in loops

* Add nested else test

* Fix bug in for else implementation

* Update tests to use assert

* Add print statements in tests
@advikkabra advikkabra deleted the nested-tests branch July 21, 2024 13:22
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