search

gchq / gaffer-tools

gaffer-tools is deprecated. Use https://github.com/gchq/gafferpy instead
Apache License 2.0
50 stars 29 forks source link

Gh 820 export import buttons tests #829

Closed macenturalxl1 closed 4 years ago

macenturalxl1 commented 4 years ago

Hi here are tests to go along side the import & export buttons.

For import(), I couldn't test cover the lines for reader.onloadend function() - do you have any recommendations for that or is acceptable to not cover it?

Also I made the mistake of pulling the latest changes from develop branch instead of the gh-820 branch - if you pull the latest develop branch commits in to gh-820 then you should see it's just the one test file I've created, or if you prefer I can got back and revert the latest develop changes merge in my branch to be in sync with gh-820 commits.

Or I can direct the PR to develop and put a note on to wait for gh-820 merge.

Let me know what you would like me to do? Also any feedback, thanks

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

t11947 commented 4 years ago

For import(), I couldn't test cover the lines for reader.onloadend function() - do you have any recommendations for that or is acceptable to not cover it?

Ideally I'd like this to be covered. Feel free to refactor the code to make it easier to test.

t11947 commented 4 years ago

Also I made the mistake of pulling the latest changes from develop branch instead of the gh-820 branch - if you pull the latest develop branch commits in to gh-820 then you should see it's just the one test file I've created, or if you prefer I can got back and revert the latest develop changes merge in my branch to be in sync with gh-820 commits.

Or I can direct the PR to develop and put a note on to wait for gh-820 merge.

We've resolved this. You're now merging into develop.

macenturalxl1 commented 4 years ago

We've resolved this. You're now merging into develop.

Thanks

macenturalxl1 commented 4 years ago

For import(), I couldn't test cover the lines for reader.onloadend function() - do you have any recommendations for that or is acceptable to not cover it?

Ideally I'd like this to be covered. Feel free to refactor the code to make it easier to test.

Agreed - I'll refactor the code so the block is testable...

macenturalxl1 commented 4 years ago

Hi - ready for next review.

CSS comment on the html view - I've replaced the inline styling and enhanced the design using existing CSS and tags available so it is more consitent with the overall application design. It also means the styling is now dynamic, if multiple themes are added in future (i.e. dark mode, other custom CSS). Let me know what you think of the improved design - it is in a state that is easy to change to add your own ideas.

Test coverage - I tried multiple ways to cover the block, however found the cleanest and safest way to both test it and allow the FileReader onload to access it was to add it to the $scope. I tried adding it as a component or binding it to this etc. but required more complex and unreadable implementation. Adding to scope just seemed to be the better option.

Let me know what you think?