aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David McLaughlin <da...@dmclaughlin.com>
Subject Re: Review Request 62607: Replace Preact and custom testing with React + Enzyme
Date Wed, 27 Sep 2017 21:26:48 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message