-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Part of #3928 [Testing Examples] Add First Class Python Support #4166
Part of #3928 [Testing Examples] Add First Class Python Support #4166
Conversation
CC @jodersky to take a look |
it's been 3 days since completion But no Review 🥺 |
Hey Himanshu, I've been with family over the christmas holidays, and before
then I was sick. So unfortunately I haven't really had much time to spend
on mill. Note however that I haven't forgotten about this, and I appreciate
your patience
…On Fri, Dec 27, 2024, 10:43 Himanshu Mahajan ***@***.***> wrote:
it's been 3 days since completion But no Review 🥺
—
Reply to this email directly, view it on GitHub
<#4166 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHV3JCKUBNROYOCAMKVVTD2HUOMFAVCNFSM6AAAAABT5SSYXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRTGUYTKNJXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@jodersky No Worries, Sir we can wait till 2025 start, after New Year we can work on this.... BTW Happy New Year ✨ Thanks for Your Response 🤞 |
Hey @himanshumahajan138 , happy new year! Just got back |
@jodersky Sir i dont think this failing python testcase should fail in general what u say ?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience @himanshumahajan138 I finally got to reviewing your PR.
It think it's looking good, but there are still a few changes to be made before we can merge it
example/pythonlib/testing/1-test-suite/foo/test/src/test_foo.py
Outdated
Show resolved
Hide resolved
|
||
object test extends PythonTests with TestModule.Unittest { | ||
def moduleDeps = super.moduleDeps ++ Seq(bar.test) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to show pythonDeps
used here too, since it's mentioned in the text above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup why not !
i will use the inbuilt libs which we are also using in the backend just for demo purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing i want to mention is that if i added some library in the foo object then it will be available for test also(you know this)
so why we should add some libs demo in test ?
Altough i have added still for demo Purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to add a python dependency to the test and use that.
so why we should add some libs demo in test ?
The reason is to show how a user can use a library in tests (for example a helper function), but not depend on it in the main module
Once that is done, you have green light to merge from me. However I'd like @lihaoyi to sign off first due to the failing CI test which appears to be unrelated, but I don't want to take any chances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sir let choose one thing and add something you too want...
- Use some library in the test which we will use only in test object and show that example on top of this
- Add some test library in the test (maybe PyTest) and use that to perform test
Add your requirement in these with instruction please
Moreover, i think that this failing testcase is Flaky and will get passed on multiple runs (on its own)
@jodersky its great to see you back ✨ Thanks for Reviewing! let me Update the Code and then Finalize this and move ahead to further support 🤞 |
@jodersky Sir All Done ✨ Let's get up in Contribution Leaderboard 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! If you can show pythonDeps
being used in tests then I think this is good to go.
Not sure what's up with the failing android test. Let's retry once more
Actually Mill's CI have some type of personal issue(grudge) with me i.e why it always fails irrelevantly in my PR's 😂 |
@jodersky Sir, please see if i addressed your concern... |
example/pythonlib/testing/1-test-suite/bar/test/src/test_bar.py
Outdated
Show resolved
Hide resolved
example/pythonlib/testing/1-test-suite/bar/test/src/test_bar.py
Outdated
Show resolved
Hide resolved
@lihaoyi Updated Based on Your Reviews! |
Are we Merging ??? |
Pull Request
Adds First Class Python Support [Testing Examples]
Part of #3928
Description
Testing Examples for Python
1-test-suite
,2-test-deps
,3-integration-suite
ExamplesRelated Issues
Checklist
Status
WIP & Require Review!!!