Fix Profile.render() to return str. It's expected that .render()
methods return str but in py3 it started returning bytes.
Also fix reading/writing profile as string when it should be bytes.
Differential D94418
[LNT] Python 3 support: fix several bytes/str inconsistencies danilaml on Jan 11 2021, 8:06 AM. Authored by
Details
Fix Profile.render() to return str. It's expected that .render() Also fix reading/writing profile as string when it should be bytes.
Diff Detail
Event Timeline
Comment Actions Could you add a test for this method as well? Clearly the testsuite did not exercise this code or it would have been caught earlier. Comment Actions I'll try to come up with a test case. I thought there were already tests for profile data, so I need to look into why they didn't catch it.
Comment Actions Added a test. Btw, I've also found that one lit test (testing/profilev1impl.py) currently fails with Python 3 (should be trivial to fix), but this raises the question - are the tests still being run under py2 only?
Comment Actions Tests are run in Python 3 but this test simply does not seem to be run for some reason. I tried to change one of the assertTrue for an assertFalse inside and neither Python2 nor Python3 testing failed.
Comment Actions I was wrong, not only is the test run but it also fails if one uses the latest source. With the old source nothing would get run anything passed the import. I've tracked it down to the import of cPerf in lnt/testing/profile/perf.py. This must have been fixed by @tnfchris f98bc1b3018248fbe551b0e7491ac5ba388216d5 patch. Comment Actions Apply the proposed edit to profile.py as well as the following two changes to fix the profilev1impl test error:
I'm not sure whether we should do a separate patch for that or not considering that your fix to render is also needed for the testcase to pass. You could perhaps reword your title and/or description to mention that this fixes the failing test.
Comment Actions Could you explain me what does the test you added do? I'm not familiar with lnt runtest and its option nor with all the lit thingy. Comment Actions Fixed tests. Btw, I've also found several places where files are opened without being closed (i.e. open(...).read(N) == b'...', some NamedTemporary usages). It's not a big issue, but a candidate for a cleanup patch.
Comment Actions Be my guest :-) I'm trying to clean up all mypy and flake8 warnings
Comment Actions lit here is used in very simple way: it just reads the file and runs all the commands in RUN: (the test fails if the command exit code is non-zero). Beside that I only use some pre-defined substitutions (%s - current file, %t - temporary name/file, %{shared_inputs} - path to SharedInputs). As for runtest: I've copied the beginning of the test_suite-profile.shtest test, that runs the test-suite under fake lit fake-lit-profile which just copies the results.json to the designated place, but I've removed the exec-multisample option (since i want the test only imported once). I thought about simplifying the test by only checking that report.json contains valid BASE64 Data, but I think that this way it's more foolproof (I.e. it doesn't rely on the way the data is marshaled - just checks that it can successfully "roundtrip"). Comment Actions I've seen it landed on the upstream branch rather than the main branch. Was that a mistake? Comment Actions I've tried applying your patch and its test fail for me.
Comment Actions +$ ":" "RUN: at line 2" +$ "rm" "-rf" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.SANDBOX" +$ ":" "RUN: at line 3" +$ "lnt" "runtest" "test-suite" "--sandbox" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.SANDBOX" "--no-timestamp" "--test-suite" "/home/thomasp/repos/lnt/tests/ru ntest/Inputs/test-suite-cmake" "--cc" "/home/thomasp/repos/lnt/tests/../tests/SharedInputs/FakeCompilers/clang-r154331" "--use-cmake" "/home/thomasp/repos/lnt/tests/runtest/Inputs/test-suite-cmake/fake-cm ake" "--use-make" "/home/thomasp/repos/lnt/tests/runtest/Inputs/test-suite-cmake/fake-make" "--use-lit" "/home/thomasp/repos/lnt/tests/runtest/Inputs/test-suite-cmake/fake-lit-profile-import" "--use-perf= all" "-j2" "--verbose" +$ ":" "RUN: at line 15" +$ "rm" "-rf" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.DB" +$ ":" "RUN: at line 16" +$ "lnt" "create" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.DB" +$ ":" "RUN: at line 17" +$ "lnt" "import" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.DB" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.SANDBOX/bu ild/report.json" "--show-sample-count" +$ ":" "RUN: at line 19" +$ "python" "/home/thomasp/repos/lnt/tests/runtest/test_suite-profile-import.py" "/home/thomasp/repos/lnt/test_run_tmp/runtest/Output/test_suite-profile-import.py.tmp.DB" +# command stderr: +Traceback (most recent call last): + File "/home/thomasp/repos/lnt/tests/runtest/test_suite-profile-import.py", line 26, in <module> + profile = glob.glob('%s/data/profiles/*.lntprof' % sys.argv[1])[0] +IndexError: list index out of range Comment Actions Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment. Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine. Comment Actions I use lit directly (lit -svv ./tests) which what I understand tox does as well. I've rerun all tests using tox and the only test that failed was server/ui/V4Pages.py (I believe it's because I've already had an lnt instance running and that test doesn't account for that possibility). Comment Actions Are you testing your patch on top of 042938b5c332fa63f1fefdd7116f39e161b8c51b? My guess is you are testing on a commit prior to f98bc1b3018248fbe551b0e7491ac5ba388216d5 which prevents tests involving profile from being run. Btw, using tox with a comparison of before/after logs allows you to spot codestyle issues added by your patch like the double glob import I pointed out. Best regards, Thomas Comment Actions Yes, I was testing upon that (otherwise I wouldn't have caught the issue with the profilvev1 test case). I've also retested on top of the current main with the same result. Btw, I can't find a link to the lnt buildbot. I think it should already finish the testing. Comment Actions %t.err has: 2021-01-14 18:07:42 WARNING: Traceback (most recent call last): File "/home/thomasp/repos/lnt/lnt/testing/profile/perf.py", line 32, in deserialize data = cPerf.importPerf(f, nm, objdump) NameError: global name 'cPerf' is not defined I'm not sure why cPerf appears not to be installed under tox, it should build everything. tests/testing/cPerf.py does not have this problem because it skips the test if cPerf is not found. Comment Actions Got it, this is because of tests/lit.cfg which adds the source tree in PYTHONPATH. This will have priority over what is in .tox/py27 and .tox/py3 which is where the extension will be installed when running under tox. I'm guessingy you ran setup.py in develop mode which installed the cPerf extension into the repo. If I apply this change: -config.environment['PYTHONPATH'] = '%s:%s' % (build_root, src_root) +config.environment['PYTHONPATH'] = '%s' % build_root then your test passes. I'm unsure how to proceed forward from there. I presume running lit outside of tox requires the source repo to be in PYTHONPATH but then your test need to be changed not to assume cPerf is available (since it only is when lit is installed). I'm personally more enclined to prevent lit from running without lit installed because then some of the tests are skipped. What do you think? Comment Actions
Do you mean without lnt installed? If so, I agree. It makes sense to expect LNT to b fully configured/installed before running the tests. Not sure about PYTHONPATH manipulations. Go ahead, if it fixes running under tox. I see you've already fixed the issue with LNT :: server/ui/V4Pages.py I had. Comment Actions Sorry, I'm not aware of a buildbot for LNT. I did not even know there was one before. Comment Actions
I remember triggering it sometime in 2019. Don't know what happened to it since then. Maybe @cmatthews remembers. Also, don't know if you can, but it'd be great to finally setup LNT (and test-suite for that matter) repositories in phabricator (so it'd close submitted reviews automatically). |