You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by David McLaughlin <da...@dmclaughlin.com> on 2017/09/27 15:04:32 UTC

Review Request 62607: Replace Preact and custom testing with React + Enzyme

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/
-----------------------------------------------------------

Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Repository: aurora


Description
-------

Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.

Side-effects:

* The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
* Deleted custom (and questionable) shallow renderer.
* The download time will be reduced due to not having to download PhantomJS.


Diffs
-----

  ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
  ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
  ui/src/__mocks__/react.js PRE-CREATION 
  ui/src/main/js/components/Pagination.js PRE-CREATION 
  ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
  ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
  ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
  ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
  ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
  ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
  ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
  ui/test-setup.js PRE-CREATION 
  ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 


Diff: https://reviews.apache.org/r/62607/diff/1/


Testing
-------

Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:

$ npm run lint
$ npm test

I also tested the UI in my Vagrant image.


Thanks,

David McLaughlin


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 4:28 p.m., Joshua Cohen wrote:
> > FWIW there's also a version of React 15 that's been licensed under MIT: https://facebook.github.io/react/blog/2017/09/25/react-v15.6.2.html

Ah, good to know. I think this is the correct approach anyway. Reactable would eventually block us from upgrading if we wanted to use some other new library.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186425
-----------------------------------------------------------


On Sept. 27, 2017, 3:04 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 3:04 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/1/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186425
-----------------------------------------------------------



FWIW there's also a version of React 15 that's been licensed under MIT: https://facebook.github.io/react/blog/2017/09/25/react-v15.6.2.html

- Joshua Cohen


On Sept. 27, 2017, 3:04 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 3:04 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/1/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186435
-----------------------------------------------------------


Ship it!




Master (474320b) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Sept. 27, 2017, 4:50 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 4:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/2/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Joshua Cohen <jc...@apache.org>.

> On Sept. 27, 2017, 8:35 p.m., Kai Huang wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 93 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837592#file1837592line93>
> >
> >     Should this be extracted into renderer? So that we don't need to pass isTable flags?
> 
> David McLaughlin wrote:
>     I added a comment that describes why the user must provide this. You cannot return a list of elements - you have to return a single container. So Pagination needs to know what to wrap your items with.

Another "fwiw", I think in React 16 you *can* return a list of elements: https://facebook.github.io/react/blog/2017/09/26/react-v16.0.html#new-render-return-types-fragments-and-strings (note I haven't actually reviewed the code in question to see whether or not it makes sense in this situation).


- Joshua


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186469
-----------------------------------------------------------


On Sept. 27, 2017, 9:31 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 9:31 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/4/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 8:35 p.m., Kai Huang wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837592#file1837592line75>
> >
> >     nit, the variable name here collide with the `currentPage` attribute in Pages component. s/currentPage/currentPageItems/

Done.


> On Sept. 27, 2017, 8:35 p.m., Kai Huang wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 87 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837592#file1837592line87>
> >
> >     s/maxPages/pageRange/

Max pages is an accurate description - it's the maximum of pages to show (but for simplicity not always the actual number we show in cases where there are odd numbers or other imbalances).


> On Sept. 27, 2017, 8:35 p.m., Kai Huang wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 93 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837592#file1837592line93>
> >
> >     Should this be extracted into renderer? So that we don't need to pass isTable flags?

I added a comment that describes why the user must provide this. You cannot return a list of elements - you have to return a single container. So Pagination needs to know what to wrap your items with.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186469
-----------------------------------------------------------


On Sept. 27, 2017, 5:51 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 5:51 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/3/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 8:35 p.m., Kai Huang wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 93 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837592#file1837592line93>
> >
> >     Should this be extracted into renderer? So that we don't need to pass isTable flags?
> 
> David McLaughlin wrote:
>     I added a comment that describes why the user must provide this. You cannot return a list of elements - you have to return a single container. So Pagination needs to know what to wrap your items with.
> 
> Joshua Cohen wrote:
>     Another "fwiw", I think in React 16 you *can* return a list of elements: https://facebook.github.io/react/blog/2017/09/26/react-v16.0.html#new-render-return-types-fragments-and-strings (note I haven't actually reviewed the code in question to see whether or not it makes sense in this situation).

Ah, good to know - that's an excellent addition. In this case I do still need to know it's a table so I can wrap the pagination navigation in a tr, so will leave as-is. But I'll make sure the comment is modified to reflect this in a follow-up review.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186469
-----------------------------------------------------------


On Sept. 27, 2017, 9:31 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 9:31 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/4/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Kai Huang <te...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186469
-----------------------------------------------------------




ui/src/main/js/components/Pagination.js
Lines 75 (patched)
<https://reviews.apache.org/r/62607/#comment263020>

    nit, the variable name here collide with the `currentPage` attribute in Pages component. s/currentPage/currentPageItems/



ui/src/main/js/components/Pagination.js
Lines 87 (patched)
<https://reviews.apache.org/r/62607/#comment263021>

    s/maxPages/pageRange/



ui/src/main/js/components/Pagination.js
Lines 93 (patched)
<https://reviews.apache.org/r/62607/#comment263031>

    Should this be extracted into renderer? So that we don't need to pass isTable flags?


- Kai Huang


On Sept. 27, 2017, 5:51 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 5:51 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/3/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186498
-----------------------------------------------------------



@ReviewBot retry

- Stephan Erb


On Sept. 27, 2017, 11:31 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 11:31 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/4/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186490
-----------------------------------------------------------



Master (abd6fad) is red with this patch.
  ./build-support/jenkins/build.sh

:licenseTest UP-TO-DATE
:license UP-TO-DATE
:pmdJmh
:pmdMain
:pmdTest
:testI0927 21:48:58.840 [ShutdownHook, SchedulerMain] Stopping scheduler services. 
 FAILED
:jacocoTestReport
Coverage report generated: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:jacocoTestCoverageVerification[ant:jacocoReport] Rule violated for bundle aurora: instructions covered ratio is 0.80, but expected minimum is 0.87
[ant:jacocoReport] Rule violated for bundle aurora: branches covered ratio is 0.70, but expected minimum is 0.79
 FAILED

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':test'.
> Process 'Gradle Test Executor 6' finished with non-zero exit value 137

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
==============================================================================

2: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for bundle aurora: instructions covered ratio is 0.80, but expected minimum is 0.87
  Rule violated for bundle aurora: branches covered ratio is 0.70, but expected minimum is 0.79

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
==============================================================================

* Get more help at https://help.gradle.org

BUILD FAILED in 7m 46s
50 actionable tasks: 41 executed, 9 up-to-date


I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Sept. 27, 2017, 9:31 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 9:31 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/4/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Kai Huang <te...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186485
-----------------------------------------------------------


Ship it!




Ship It!

- Kai Huang


On Sept. 27, 2017, 9:31 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 9:31 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/4/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/
-----------------------------------------------------------

(Updated Sept. 27, 2017, 9:31 p.m.)


Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Changes
-------

feedback.


Repository: aurora


Description
-------

Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.

Side-effects:

* The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
* Deleted custom (and questionable) shallow renderer.
* The download time will be reduced due to not having to download PhantomJS.


Diffs (updated)
-----

  build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
  ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
  ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
  ui/src/__mocks__/react.js PRE-CREATION 
  ui/src/main/js/components/Pagination.js PRE-CREATION 
  ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
  ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
  ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
  ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
  ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
  ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
  ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
  ui/test-setup.js PRE-CREATION 
  ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 


Diff: https://reviews.apache.org/r/62607/diff/4/

Changes: https://reviews.apache.org/r/62607/diff/3-4/


Testing
-------

$ ./gradlew ui:lint
$ ./gradlew ui:test

I also tested the UI in my Vagrant image.


Thanks,

David McLaughlin


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186454
-----------------------------------------------------------


Ship it!




Master (474320b) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Sept. 27, 2017, 10:51 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 10:51 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/3/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/Pagination.js
> > Lines 3 (patched)
> > <https://reviews.apache.org/r/62607/diff/2/?file=1837566#file1837566line3>
> >
> >     nit - `maxPages` and `numPages` are a little confusing. Can we use the terminology similar to Reactable - such as `itemsPerPage` and `pageButtonLimit` or similar?

itemsPerPage isn't represented here since the range is provide to Pages. I think conceptually the tricky thing here is to realize this is a cosmetic component for returning the actual page navigation and not doing any paging itself. So I've changed the name of the component to help reflect that.


> On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/__tests__/Pagination-test.js
> > Lines 155-158 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837596#file1837596line155>
> >
> >     The order will be &laquo;, 7, 8, 9 correct?

enzyme isn't as good as my shallow renderer ;) I couldn't find a way to enforce ordering without having to match *all* properties (which is really difficult when you have props that are functions) so this is just verifying they are present.


> On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote:
> > ui/src/main/js/components/__tests__/Pagination-test.js
> > Lines 167-173 (patched)
> > <https://reviews.apache.org/r/62607/diff/3/?file=1837596#file1837596line167>
> >
> >     nit - re-order?

I actually did this intentionally to be very clear about the lack of order enforcement in the test assertion. But I can reorder it if needed.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186472
-----------------------------------------------------------


On Sept. 27, 2017, 5:51 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 5:51 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/3/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186472
-----------------------------------------------------------


Ship it!




LGTM. Minor comments about readability.


ui/src/main/js/components/Pagination.js
Lines 3 (patched)
<https://reviews.apache.org/r/62607/#comment263027>

    nit - `maxPages` and `numPages` are a little confusing. Can we use the terminology similar to Reactable - such as `itemsPerPage` and `pageButtonLimit` or similar?



ui/src/main/js/components/Pagination.js
Lines 3 (patched)
<https://reviews.apache.org/r/62607/#comment263044>

    nit - maxPages and numPages are a little confusing. Can we use the terminology similar to Reactable - such as itemsPerPage and pageButtonLimit or similar?



ui/src/main/js/components/__tests__/Pagination-test.js
Lines 155-158 (patched)
<https://reviews.apache.org/r/62607/#comment263042>

    The order will be &laquo;, 7, 8, 9 correct?



ui/src/main/js/components/__tests__/Pagination-test.js
Lines 167-173 (patched)
<https://reviews.apache.org/r/62607/#comment263045>

    nit - re-order?


- Santhosh Kumar Shanmugham


On Sept. 27, 2017, 10:51 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 10:51 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/3/
> 
> 
> Testing
> -------
> 
> $ ./gradlew ui:lint
> $ ./gradlew ui:test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/
-----------------------------------------------------------

(Updated Sept. 27, 2017, 5:51 p.m.)


Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Changes
-------

Upgrade node-plugin to 1.2.0 to address Gradle issues.


Repository: aurora


Description
-------

Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.

Side-effects:

* The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
* Deleted custom (and questionable) shallow renderer.
* The download time will be reduced due to not having to download PhantomJS.


Diffs (updated)
-----

  build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 
  ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
  ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
  ui/src/__mocks__/react.js PRE-CREATION 
  ui/src/main/js/components/Pagination.js PRE-CREATION 
  ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
  ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
  ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
  ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
  ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
  ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
  ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
  ui/test-setup.js PRE-CREATION 
  ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 


Diff: https://reviews.apache.org/r/62607/diff/3/

Changes: https://reviews.apache.org/r/62607/diff/2-3/


Testing (updated)
-------

$ ./gradlew ui:lint
$ ./gradlew ui:test

I also tested the UI in my Vagrant image.


Thanks,

David McLaughlin


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/
-----------------------------------------------------------

(Updated Sept. 27, 2017, 4:50 p.m.)


Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.


Changes
-------

Added some comments to Pagination.


Repository: aurora


Description
-------

Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.

Side-effects:

* The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
* Deleted custom (and questionable) shallow renderer.
* The download time will be reduced due to not having to download PhantomJS.


Diffs (updated)
-----

  ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
  ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
  ui/src/__mocks__/react.js PRE-CREATION 
  ui/src/main/js/components/Pagination.js PRE-CREATION 
  ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
  ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
  ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
  ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
  ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
  ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
  ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
  ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
  ui/test-setup.js PRE-CREATION 
  ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 


Diff: https://reviews.apache.org/r/62607/diff/2/

Changes: https://reviews.apache.org/r/62607/diff/1-2/


Testing
-------

Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:

$ npm run lint
$ npm test

I also tested the UI in my Vagrant image.


Thanks,

David McLaughlin


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186422
-----------------------------------------------------------


Ship it!




Master (474320b) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Sept. 27, 2017, 3:04 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 3:04 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/1/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Bill Farner <wf...@apache.org>.

> On Sept. 27, 2017, 9:08 a.m., Bill Farner wrote:
> > > Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle
> > 
> > I didn't witness 4.2 causing breakage, but did find other issues, patched at [62620](https://reviews.apache.org/r/62620/).  I'll be curious what's going on if that fix your setup as well.
> 
> David McLaughlin wrote:
>     To be clear: it doesn't break the build. But ./gradlew ui:test and ./gradlew ui:lint were giving false positives and then were marked as UP-TO-DATE even after changing the sources.
> 
> David McLaughlin wrote:
>     It looks like the gradle-node-plugin isn't working with 4.1:
>     
>     https://github.com/srs/gradle-node-plugin/issues/251
>     
>     And there isn't a lot of support. I don't have the bandwidth to look into the plugin. Was there a motivation for upgrading gradle to such a recent version? (4.2 was just released AFAICT)

I upgraded to get JDK 9 support (introduced in 4.1).  However, there isn't urgency to this.  Reverting to 4.0.2 should be fine if that addresses the issues.  Otherwise, i'd like to be >= 3.4 to keep the jacoco coverage check introduced.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186421
-----------------------------------------------------------


On Sept. 27, 2017, 9:50 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 9:50 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/2/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 4:08 p.m., Bill Farner wrote:
> > > Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle
> > 
> > I didn't witness 4.2 causing breakage, but did find other issues, patched at [62620](https://reviews.apache.org/r/62620/).  I'll be curious what's going on if that fix your setup as well.
> 
> David McLaughlin wrote:
>     To be clear: it doesn't break the build. But ./gradlew ui:test and ./gradlew ui:lint were giving false positives and then were marked as UP-TO-DATE even after changing the sources.

It looks like the gradle-node-plugin isn't working with 4.1:

https://github.com/srs/gradle-node-plugin/issues/251

And there isn't a lot of support. I don't have the bandwidth to look into the plugin. Was there a motivation for upgrading gradle to such a recent version? (4.2 was just released AFAICT)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186421
-----------------------------------------------------------


On Sept. 27, 2017, 4:50 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 4:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/2/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by David McLaughlin <da...@dmclaughlin.com>.

> On Sept. 27, 2017, 4:08 p.m., Bill Farner wrote:
> > > Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle
> > 
> > I didn't witness 4.2 causing breakage, but did find other issues, patched at [62620](https://reviews.apache.org/r/62620/).  I'll be curious what's going on if that fix your setup as well.

To be clear: it doesn't break the build. But ./gradlew ui:test and ./gradlew ui:lint were giving false positives and then were marked as UP-TO-DATE even after changing the sources.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186421
-----------------------------------------------------------


On Sept. 27, 2017, 3:04 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 3:04 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/1/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme

Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186421
-----------------------------------------------------------



> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle

I didn't witness 4.2 causing breakage, but did find other issues, patched at [62620](https://reviews.apache.org/r/62620/).  I'll be curious what's going on if that fix your setup as well.

- Bill Farner


On Sept. 27, 2017, 8:04 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 8:04 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Replace Preact and custom testing with React + jest + Enzyme now that they are MIT licensed.
> 
> Side-effects:
> 
> * The Reactable library is not compatible with React 16.0 (the MIT licensed version we are forced to use) and attempts to flag this started as early as June this year. But the author seems to have abandoned the library. So I had to create my own Pagination class. I found myself repeatedly using Reactable in places where I just wanted pagination (for lists of divs) anyway, so maybe this is a good thing.
> * Deleted custom (and questionable) shallow renderer.
> * The download time will be reduced due to not having to download PhantomJS.
> 
> 
> Diffs
> -----
> 
>   ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 
>   ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 
>   ui/src/__mocks__/react.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js 32595601aae06730f537d37d95be2e746f779103 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 18af0fe6c360f375fb0fa1694304d821041d9e16 
>   ui/src/main/js/components/__tests__/Home-test.js 8e6bc09050148ea9711dbfe6f52b83c8210262be 
>   ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 
>   ui/src/main/js/utils/ShallowRender.js 52e8bb259264e7dd47ee97cec35553acc147e818 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 
>   ui/src/main/sass/components/_tables.scss 58f176cffcdae34de7c776e31b3f0342efa3024f 
>   ui/test-setup.js PRE-CREATION 
>   ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 
> 
> 
> Diff: https://reviews.apache.org/r/62607/diff/1/
> 
> 
> Testing
> -------
> 
> Unfortunately the Gradle 4.2 upgrade seems to have broken running builds from gradle (will attempt to fix in another ticket)... So I had to manually verify with:
> 
> $ npm run lint
> $ npm test
> 
> I also tested the UI in my Vagrant image.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>