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

Replace lambdas with functions #652

Merged
merged 1 commit into from Dec 30, 2019
Merged

Replace lambdas with functions #652

merged 1 commit into from Dec 30, 2019

Conversation

Solomon1732
Copy link
Contributor

@Solomon1732 Solomon1732 commented Dec 27, 2019

Replace lambdas with functions. Remove a use of old_round (total removal depends on Tribler/tribler#5024 and Tribler/anydex-core#31). Other minor refractoring: removing the redundant u prefix from strings; remove cast_to_* functions when it is immediately clear how to manipulate the data.

Fixes #651.

@Solomon1732
Copy link
Contributor Author

The first commit is me not properly merging commits from upstream. I will remove it when I squash the merges. Currently the PR is WIP.

@Tribler Tribler deleted a comment from tribler-ci Dec 27, 2019
@Solomon1732
Copy link
Contributor Author

The command git config -f .gitmodules --get-regexp ^submodule\.(.+)\.url # timeout=10 errs with ERROR: No submodules found. on all tests. What is this error?

@devos50
Copy link
Contributor

devos50 commented Dec 27, 2019

That ERROR message is non-critical and only indicates that there are no submodules. Note that the Jenkins job still proceeds and starts to run the tests.

This error is most likely related to something in the PR.

@Solomon1732
Copy link
Contributor Author

Even though the error isn't critical the gui tests don't run (probably because they depend on the basic tests).

@devos50
Copy link
Contributor

devos50 commented Dec 27, 2019

@Solomon1732 For IPv8, there are no GUI tests for the IPv8 directory.

The Jenkins log shows the following:

+ [ asyncio = master ]
+ python3 -m unittest discover -v ipv8.test
test_get_attributes (ipv8.test.REST.attestationendpoint.test_attestation_endpoint.TestAttestationEndpoint) ... Received unknown message: 195 from (65.67.22.77, 18003)
Received unknown message: 195 from (65.67.22.77, 18003)
Received unknown message: 195 from (98.229.210.78, 32042)
Received unknown message: 195 from (98.229.210.78, 32042)
Build step 'Execute shell' marked build as failure

There's no clear error here unfortunately. I will run them on my machine in a moment to see if I can see what's wrong.

Also, this PR seems to be on the asyncio branch. I assume that's your intention?

@qstokkink
Copy link
Collaborator

@devos50 Is nosetests3 installed on our infrastructure? That properly outputs logging on failure.

@devos50
Copy link
Contributor

devos50 commented Dec 27, 2019

@qstokkink yeah nosetests3 should be installed 👍

@qstokkink
Copy link
Collaborator

@Solomon1732 it's probably failing because decode does not do the same as cast_to_chr:

> b"\x80".decode()
Traceback (most recent call last):
  File "main.py", line 1, in <module>
    b"\x80".decode()
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 0: invalid start byte

Same goes for cast_to_bin and encode.

@qstokkink
Copy link
Collaborator

It might be how your Git branch is based. Try rebasing to the latest asyncio.

@Solomon1732
Copy link
Contributor Author

Also, this PR seems to be on the asyncio branch. I assume that's your intention?

Yes, yes it is.

It might be how your Git branch is based. Try rebasing to the latest asyncio.

Ah, ok. Will try

@Solomon1732
Copy link
Contributor Author

it's probably failing because decode does not do the same as cast_to_chr:

Got it. Though cast_to_chr results in this

>>> def cast_to_chr(obj):
    return "".join(chr(c) for c in obj)

>>> cast_to_chr(b"\x80")
'\x80'

Is the behavior intentional?

@Solomon1732
Copy link
Contributor Author

Odd. Still failing. If I delete the branch locally and on remote, will the PR still stay? I guess not, but worth asking.

@Solomon1732
Copy link
Contributor Author

I tried

git checkout origin/asyncio
git branch -f asyncio
git checkout asyncio
git branch -f f_refractor_lambda
git checkout f_refractor_lambda

but that rogue commit still appears in the PR for some reason

@Solomon1732
Copy link
Contributor Author

Even tried: delete local branch; create new f_refractor_lambda branch; push only desired change into branch; git push -f --set-upstream origin f_refractor_lambda. Still that rogue commit appears.

@Solomon1732
Copy link
Contributor Author

Considering the tests fail here and not in #655 it makes me wonder what in this PR fails the tests...

ipv8/util.py Outdated Show resolved Hide resolved
@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Dec 29, 2019

> git config -f .gitmodules --get-regexp ^submodule.(.+).url # timeout=10
ERROR: No submodules found.

I am confounded as to why it keeps failing on it here...

@Solomon1732
Copy link
Contributor Author

Solomon1732 commented Dec 29, 2019

It doesn't even build. Odd...

Test the (GET: attributes) request type ... Build step 'Execute shell' marked build as failure
Started calculate disk usage of build
Finished Calculation of disk usage of build in 0 seconds
Started calculate disk usage of workspace
Finished Calculation of disk usage of workspace in 0 seconds
Notifying upstream projects of job completion
Unable to get pull request builder trigger!!

Edit: taken from https://jenkins-ci.tribler.org/job/ipv8/job/test_ipv8_PR_unittests_linux_py36/1133/console

@Solomon1732
Copy link
Contributor Author

This submodule error here doesn't seem to be the reason, since the tests of that particular iteration passed for #655.

@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

retest this please

@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

I just disabled the recursive submodule operation of the IPv8 PR testers. Since IPv8 does not have any submodules, this step was not required 👍

@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

I tried to run the tests in the asyncio branch on my computer, using the exact same script as on Jenkins, but it runs for me. I suspect that it is related to this PR somehow :/

@Solomon1732
Copy link
Contributor Author

Test the (GET: attributes) request type ... Build step 'Execute shell' marked build as failure
Started calculate disk usage of build
Finished Calculation of disk usage of build in 0 seconds
Started calculate disk usage of workspace
Finished Calculation of disk usage of workspace in 0 seconds
Notifying upstream projects of job completion
Unable to get pull request builder trigger!!

Still can't get a pull request for some reason. Why... Maybe someone can look deeper on what happens with Jenkins behind the scenes?

@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

When running the tests with this PR on my machine:

iMac-van-MA:py-ipv8 martijndevos$ nosetests -v ipv8/test/
Test the (GET: attributes) request type ... The test-suite locked up! Force quitting! Thread dump:
THREAD#4602377664
|   File "/usr/local/bin/nosetests", line 8, in <module>
|       sys.exit(run_exit())
|   File "/usr/local/lib/python3.7/site-packages/nose/core.py", line 121, in __init__
|       **extra_args)
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/main.py", line 101, in __init__
|       self.runTests()
|   File "/usr/local/lib/python3.7/site-packages/nose/core.py", line 207, in runTests
|       result = self.testRunner.run(self.test)
|   File "/usr/local/lib/python3.7/site-packages/nose/core.py", line 62, in run
|       test(result)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 178, in __call__
|       return self.run(*arg, **kw)
|   File "/usr/local/lib/python3.7/site-packages/nose/suite.py", line 225, in run
|       test(orig)
|   File "/usr/local/lib/python3.7/site-packages/nose/case.py", line 46, in __call__
|       return self.run(*arg, **kwarg)
|   File "/usr/local/lib/python3.7/site-packages/nose/case.py", line 134, in run
|       self.runTest(result)
|   File "/usr/local/lib/python3.7/site-packages/nose/case.py", line 152, in runTest
|       test(result)
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 676, in __call__
|       return self.run(*args, **kwds)
|   File "/usr/local/lib/python3.7/site-packages/asynctest/case.py", line 297, in run
|       self._run_test_method(testMethod)
|   File "/usr/local/lib/python3.7/site-packages/asynctest/case.py", line 354, in _run_test_method
|       self.loop.run_until_complete(result)
|   File "/usr/local/lib/python3.7/site-packages/asynctest/case.py", line 224, in wrapper
|       return method(*args, **kwargs)
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 570, in run_until_complete
|       self.run_forever()
|   File "/usr/local/lib/python3.7/site-packages/asynctest/case.py", line 224, in wrapper
|       return method(*args, **kwargs)
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 538, in run_forever
|       self._run_once()
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 1746, in _run_once
|       event_list = self._selector.select(timeout)
|   File "/usr/local/lib/python3.7/site-packages/asynctest/selector.py", line 299, in select
|       return self._selector.select(timeout)
|   File "/usr/local/Cellar/python/3.7.6/Frameworks/Python.framework/Versions/3.7/lib/python3.7/selectors.py", line 558, in select
|       kev_list = self._selector.control(None, max_ev, timeout)

@Solomon1732
Copy link
Contributor Author

I tried to run the tests in the asyncio branch on my computer, using the exact same script as on Jenkins, but it runs for me. I suspect that it is related to this PR somehow :/

I'm thinking of closing this PR and starting from scratch, considering it's only this PR which is so problematic. But I'll wait for #655 before attempting to delete the repo and starting again. This one is small anyway.

@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

@Solomon1732 the error seems to be in cast_to_bin. When restoring that method to the lambda, the tests run.

@Solomon1732
Copy link
Contributor Author

Ah, then it might be because of the obj.encode() part. I removed it to see what happens.

@Solomon1732
Copy link
Contributor Author

YES! Fina-freaking-ly! Thank you so much @devos50 !!! 😄

Remove `old_round` functionality since it is no longer necessary.
@devos50
Copy link
Contributor

devos50 commented Dec 29, 2019

Nice! 😄

@Solomon1732 Solomon1732 marked this pull request as ready for review December 29, 2019 21:51
@Solomon1732 Solomon1732 changed the title WIP: Replace lambdas with functions and other minor refractoring READY: Replace lambdas with functions Dec 29, 2019
Copy link
Collaborator

@qstokkink qstokkink left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@qstokkink qstokkink changed the title READY: Replace lambdas with functions Replace lambdas with functions Dec 30, 2019
@qstokkink qstokkink merged commit 988314b into Tribler:asyncio Dec 30, 2019
@Solomon1732 Solomon1732 deleted the f_refractor_lambda branch December 30, 2019 09:44
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.

None yet

3 participants