Resolve warnings #317 & and Fix Issue: #355 #399

amit-chaudhari1 · 2021-03-26T13:16:06Z

This commit will hopefully close issues #317 and #355.

I did not create tests for layout.py since it looks like a helper in actors.py.
I could say the same about convert.py but even then, I tried creating basic tests.
If these files grow in future, they might need their own tests.

As for #355, it's a really simple check just before we fetch the url.

Thank you for your time!
I'd appreciate any feedback...

amit-chaudhari1 · 2021-03-26T14:28:25Z

These Checks seem to be failing cause I assumed matplotlib is installed. It's not.
I'll push another commit that solves this, soon.

Effects of sleep deprivation

codecov · 2021-03-26T14:59:10Z

Codecov Report

Merging #399 (f87206e) into master (068f2e9) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
+ Coverage   89.00%   89.04%   +0.04%     
==========================================
  Files          23       23              
  Lines        5376     5388      +12     
  Branches      695      695              
==========================================
+ Hits         4785     4798      +13     
  Misses        411      411              
+ Partials      180      179       -1

Impacted Files	Coverage Δ
fury/window.py	`78.75% <100.00%> (ø)`
fury/actor.py	`92.95% <0.00%> (+0.04%)`	⬆️
fury/primitive.py	`91.78% <0.00%> (+0.23%)`	⬆️
fury/convert.py	`80.00% <0.00%> (+6.66%)`	⬆️

amit-chaudhari1 · 2021-03-26T17:34:43Z

Uwa!!! help me please,
The failing CI checks are not giving me any good information...

CI is important, but in my case it takes me more time to notice and rectify my mistakes that CI catches, than it takes me to do the maintenance work... ;-;

even on the Previous PR, I could not pass all the CI checks...

skoudoro

Hi @amitchaudhari9121,

Thank you for fixing these 2 issues! overall, it looks good. It just needs some reshaping.

Thank you

skoudoro · 2021-03-29T15:13:24Z

fury/tests/test_utils.py

+    x0 = x_indices.astype(np.int64)
+    y0 = y_indices.astype(np.int64)
+    z0 = z_indices.astype(np.int64)


int instead of np.int64. it does not need to force 64bit

skoudoro · 2021-03-29T15:14:58Z

fury/tests/test_convert.py

+    with TemporaryDirectory() as tmpdir:
+        fname = os.path.join(tmpdir, 'tmp.png')
+        dpi = 100
+        fig.savefig(fname, dpi=dpi, transparent=True)
+        arr1 = load_image(fname)
+        npt.assert_array_equal(arr1, arr2)


Thanks!

I've (kindof) picked it up from the code you wrote 😅
This just tests the exact way its written in test_actors.py

But it's not redundant code. if something breaks in future. (or we add more conversion definitions) this will be useful...

skoudoro · 2021-03-29T15:16:10Z

fury/tests/test_convert.py

+    # plt.subplot(131)
+    # plt.bar(names, values)
+    # plt.subplot(132)
+    # plt.scatter(names, values)
+    # plt.subplot(133)
+    # plt.plot(names, values)
+    # plt.suptitle('Categorical Plotting')


why is it commented? your figure seems empty

The test failed for some reason when I added these args,

I think I could just test an empty figure, and confirm it works.
will that be the case?

I'm not sure...

I don't understand enough about matplotlib, so I can't reliably say why it fails when I uncomment it...
I'll keep researching & hopefully find a fix.

Any suggestions? any idea why this might be failing? any guess is appreciated...

See below the test updated. I admit that it was tricky., you had to update the matplotlib_figure_to_numpy option and the fig.savefig options to make sure that both images have the same size and orientation.

fig = plt.figure(figsize=(9, 3)) plt.subplot(131) plt.bar(names, values) plt.subplot(132) plt.scatter(names, values) plt.subplot(133) plt.plot(names, values) plt.suptitle('Categorical Plotting') arr2 = matplotlib_figure_to_numpy(fig, transparent=False, flip_up_down=False) with TemporaryDirectory() as tmpdir: fname = os.path.join(tmpdir, 'tmp.png') fig.savefig(fname, transparent=False, bbox_inches='tight', pad_inches=0) arr1 = load_image(fname) npt.assert_array_equal(arr1, arr2)

skoudoro · 2021-03-29T15:16:59Z

fury/tests/test_convert.py

+    # plt.subplot(133)
+    # plt.plot(names, values)
+    # plt.suptitle('Categorical Plotting')
+    arr2 = matplotlib_figure_to_numpy(fig, transparent=True)


for more control in your test, use transparent=False

skoudoro · 2021-03-29T15:18:14Z

docs/source/ext/github_tools.py

+    # This was pointed out as a Security issue in bandit.
+    # please look at issue #355,
+    # we fixed it, but the bandit warning might remain,
+    # need to suppress it manually ¯\_(ツ)_/¯.


to convert to a complete docstring with a NOTE section

skoudoro · 2021-03-29T15:21:39Z

docs/source/ext/github_tools.py

+        if url.lower().startswith('http'):
+            f = urlopen(req)


What happens if it does not start with http? here a proposition:

if not url.lower().startswith('http'): msg = "custom message" raise ValueError(msg) f= urlopen(req)

The ValueError will need to be tested with npt.assert_raises

Nice!
It didn't even cross my mind !!!
I will change in next commit, thank you for this.

skoudoro · 2021-03-29T15:26:05Z

concerning the tests, no worries. We will need to make some of them more robust

skoudoro

Hi @amitchaudhari9121,

See below for some suggestions to update your test. After those update, I think your PR will be ready to go.
Thanks!

skoudoro · 2021-03-30T14:03:43Z

docs/source/ext/github_tools.py

+    '''This was pointed out as a Security issue in bandit.
+    please look at issue #355,
+    we fixed it, but the bandit warning might remain,
+    need to suppress it manually (just ignore it)'''


The docstring is incomplete. Please, use double-quotes and follow the standard of our docstring. As an example, you can look here

How do I stop making these types of mistakes?

It might be annoying for you, I'm sorry for the inconvinience.

My VScode does not pick them up ( I've been using pycodestyle as a linter)

You did not address my comment above. Please, look at the example cited above and complete the docstring.
As suggested, the text should be in a NOTE

skoudoro · 2021-03-30T14:24:09Z

fury/tests/test_convert.py

+    # plt.subplot(131)
+    # plt.bar(names, values)
+    # plt.subplot(132)
+    # plt.scatter(names, values)
+    # plt.subplot(133)
+    # plt.plot(names, values)
+    # plt.suptitle('Categorical Plotting')


See below the test updated. I admit that it was tricky., you had to update the matplotlib_figure_to_numpy option and the fig.savefig options to make sure that both images have the same size and orientation.

fig = plt.figure(figsize=(9, 3)) plt.subplot(131) plt.bar(names, values) plt.subplot(132) plt.scatter(names, values) plt.subplot(133) plt.plot(names, values) plt.suptitle('Categorical Plotting') arr2 = matplotlib_figure_to_numpy(fig, transparent=False, flip_up_down=False) with TemporaryDirectory() as tmpdir: fname = os.path.join(tmpdir, 'tmp.png') fig.savefig(fname, transparent=False, bbox_inches='tight', pad_inches=0) arr1 = load_image(fname) npt.assert_array_equal(arr1, arr2)

skoudoro · 2021-03-31T05:32:48Z

Thank you for working on this @amitchaudhari9121. merging

amit-chaudhari1 added 5 commits March 24, 2021 20:13

Fix test_utils.py warning

8e455c7

This should fix (#355):

73453ee

test_actors.py: Fix 1 unecessary warning

c2706b3

Basic Convert.py testing

ed96c17

pep8 fix

a92ddf9

amit-chaudhari1 added 3 commits March 26, 2021 20:06

CI: fix matplot lib is an Optional Module

ad8120b

forgot import in test_convert.py -_-

00c5c44

Effects of sleep deprivation

CI fix: proper imports

5905d82

skoudoro added the type:Maintenance label Mar 29, 2021

skoudoro added this to the v0.8.0 milestone Mar 29, 2021

skoudoro reviewed Mar 29, 2021

View reviewed changes

Made the suggested Changes in review.

185200e

skoudoro reviewed Mar 30, 2021

View reviewed changes

amit-chaudhari1 added 2 commits March 30, 2021 20:43

Revised changes:

d8c6173

Revised Changes

f87206e

skoudoro merged commit 86c7e42 into fury-gl:master Mar 31, 2021

This was linked to issues Mar 31, 2021

Track and fix warnings during tests. #317

Closed

[Vulnerability Bug] Used blacklisted dangerous function call that can lead to RCE #355

Closed

This was referenced Mar 31, 2021

[Vulnerability Bug] Used blacklisted dangerous function call that can lead to RCE #355

Closed

Track and fix warnings during tests. #317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve warnings #317 & and Fix Issue: #355 #399

Resolve warnings #317 & and Fix Issue: #355 #399

amit-chaudhari1 commented Mar 26, 2021

amit-chaudhari1 commented Mar 26, 2021

codecov bot commented Mar 26, 2021 •

edited

amit-chaudhari1 commented Mar 26, 2021

skoudoro left a comment

skoudoro Mar 29, 2021

skoudoro Mar 29, 2021

amit-chaudhari1 Mar 29, 2021

skoudoro Mar 29, 2021

amit-chaudhari1 Mar 29, 2021

skoudoro Mar 30, 2021

skoudoro Mar 29, 2021

skoudoro Mar 29, 2021

skoudoro Mar 29, 2021

amit-chaudhari1 Mar 29, 2021

skoudoro commented Mar 29, 2021

skoudoro left a comment

skoudoro Mar 30, 2021

amit-chaudhari1 Mar 30, 2021

skoudoro Mar 30, 2021

skoudoro Mar 30, 2021

skoudoro commented Mar 31, 2021

Resolve warnings #317 & and Fix Issue: #355 #399

Resolve warnings #317 & and Fix Issue: #355 #399

Conversation

amit-chaudhari1 commented Mar 26, 2021

This commit will hopefully close issues #317 and #355.

amit-chaudhari1 commented Mar 26, 2021

codecov bot commented Mar 26, 2021 • edited

Codecov Report

amit-chaudhari1 commented Mar 26, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro commented Mar 29, 2021

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro commented Mar 31, 2021

codecov bot commented Mar 26, 2021 •

edited