Exercise2 #4

jobegrabber · 2020-10-28T20:58:25Z

No description provided.

s0557917 · 2020-11-09T21:25:03Z

Hey, the work for the current homework looks good, but if I saw correctly, you also added all the merged files from the homework repository to the PR. It is obviously not wrong, but it makes the list of files longer, making it a bit more tedious to check. It would be better, to either merge it directly with main or create a new branch for it, thus keeping the dedicated feature branch a bit cleaner.
Hope this helps, cheers! 😀

P.S. habe euch zu Reviewers in unser Repo gesetzt, hoffe das ist jetzt in Ordnung.

jobegrabber · 2020-11-09T21:49:34Z

Hey, the work for the current homework looks good, but if I saw correctly, you also added all the merged files from the homework repository to the PR. It is obviously not wrong, but it makes the list of files longer, making it a bit more tedious to check. It would be better, to either merge it directly with main or create a new branch for it, thus keeping the dedicated feature branch a bit cleaner.
Hope this helps, cheers! grinning

P.S. habe euch zu Reviewers in unser Repo gesetzt, hoffe das ist jetzt in Ordnung.

Yes, thanks, we should merge the new exercise into main and rebase on that!
EDIT: Done.

We're peer-reviewing with team lichtow and are fairly constrained in terms of time available. Not sure if you picked the wrong group you wanted to request a review from.
If you didn't find a group to get a review from I think you should approach @medizinmensch and @roschaefer first.

s0557917 · 2020-11-09T22:00:44Z

Hey, the work for the current homework looks good, but if I saw correctly, you also added all the merged files from the homework repository to the PR. It is obviously not wrong, but it makes the list of files longer, making it a bit more tedious to check. It would be better, to either merge it directly with main or create a new branch for it, thus keeping the dedicated feature branch a bit cleaner.
Hope this helps, cheers! grinning
P.S. habe euch zu Reviewers in unser Repo gesetzt, hoffe das ist jetzt in Ordnung.

Yes, thanks, we should merge the new exercise into main and rebase on that!
EDIT: Done.

We're peer-reviewing with team lichtow and are fairly constrained in terms of time available. Not sure if you picked the wrong group you wanted to request a review from.
If you didn't find a group to get a review from I think you should approach @medizinmensch and @roschaefer first.

Allright, thanks! I'll ask around then.

noeljns · 2020-11-10T19:28:43Z

.github/workflows/node.js.yml

+
+    strategy:
+      matrix:
+        node-version: [10.x, 12.x, 14.x]


Any reason why you choose three versions instead of specifying one?

These are the three current LTS versions.

On the other PRs I said one version would be enough as you have control over the node version: Either you on your laptop or your build server will transpile your VueJS code.

But no biggie.

webapp/dojo_news/src/App.spec.js

webapp/dojo_news/src/components/NewsListItem.stories.js

powlaa

Overall I really like how you implemented all the tasks from exercise 2 👍

I'm not sure why but there seem to be a lot of files in 'archive/' that have been deleted and show up in the Files changed as well. (It was a bit annoying to click through them all when reviewing)

README.md

powlaa · 2020-11-10T19:43:28Z

webapp/dojo_news/src/App.spec.js

+    it('renders items correctly dependent on mutations of the list of NewsItems', async () => {
+        const wrapper = shallowMount(App, {
+            propsData: {
+                initialNewsListItems: [
+                    {id: 0, title: "macOS", votes: 0},
+                ]
+            }
+        });
+        expect(wrapper.find("#newslist").html()).not.toBe('<div id="newslist"></div>');
+        expect(wrapper.findAll(".newslistitem").length).toBe(1);
+
+        wrapper.vm.removeNewsListItem(0);
+
+        await Vue.nextTick();
+
+        expect(wrapper.find("#newslist").html()).toBe('<div id="newslist"></div>');
+        expect(wrapper.findAll(".newslistitem").length).toBe(0);
+
+        wrapper.vm.createNewsListItem("Test");
+        wrapper.vm.createNewsListItem("Test2");
+
+        await Vue.nextTick();
+
+        expect(wrapper.find("#newslist").html()).not.toBe('<div id="newslist"></div>');
+        expect(wrapper.findAll(".newslistitem").length).toBe(2);
+    });
+});


There are 2 things that are being tested here, adding items and removing items. I think it would be better to separate those into two test cases so that it will be more clear where a test fails.

Yes, and also DRY your test case descriptions a bit:

News list ✓ does not render any items on an initially empty list of NewsItems (29 ms) ✓ renders items on an initially filled list of NewsItems (8 ms) ✓ renders items correctly dependent on mutations of the list of NewsItems (12 ms) List Empty Message ✓ displays a message on an initially empty list of NewsItems (3 ms) ✓ displays no message on an initially non-empty list of NewsItems (4 ms) ✓ adds and removes the message on list mutations (6 ms) Reverse Order Button ✓ list order is reversed upon Reverse-Button click from initial state _descending_ (8 ms) ✓ list order is reversed upon Reverse-Button click from initial state _ascending_ (6 ms) ✓ displays the Reverse-Order-Button on an initially non empty list of NewsItems (5 ms) ✓ does not display the Reverse-Order-Button on an initially empty list of NewsItems (4 ms) ✓ Reverse-Button is hidden upon empty list (6 ms) ✓ Reverse-Button shows up again upon non-empty list (6 ms)

What about something like:

News list given no items renders empty state

While I agree with you on writing two seperate tests for two functionalities (got that fixed in the meantime), I think the structure of the tests is more of a personal preference thing. We structured our tests by feature, so we add the state to each of the test case titles. You would structure it by state, but then we would have to add the feature title to it. Or is there any other reason for our version to be bad practice?

Just make sure your test case descriptions are documenting the behaviour of your code well. It's always a little tricky because it depends on the perception of humans.

I draw lot of my knowledge from sites like https://www.betterspecs.org/

But yes, I like your idea of structuring your tests by feature.

News list emty state is displayed when list items are empty

I definitely see your point, it's a dream to be able to read code and/or test results like that. I renamed all the test cases for a better reading experience of the npm run test:unit output.

But I still think that adding another description layer would be too verbose in this case. Because for example, you would have the description "given no items" for every feature, just for one test case. And if I were to do it like that, I also had to add the descriptions "given items", "upon empty news list" and "upon non-empty news list". Everywhere, just for one test case, which in my eyes ist way too verbose.

webapp/dojo_news/src/App.vue

roschaefer

Your PR is close to perfect. Only your test cases and implementation could be improved, see my suggestions below.

You got all 9 ⭐ plus one ⭐ for your review here https://media.giphy.com/media/l2Sq5GffrCyUMEXjW/giphy.gif

That's 10/10 ⭐ well done!

README.md

webapp/dojo_news/src/App.spec.js

roschaefer · 2020-11-12T12:10:52Z

webapp/dojo_news/src/App.spec.js

+
+        let expectedIdList = [1, 0, 2, 3];
+        // or check HTML
+        let actualIdList = wrapper.vm.sortedNewsListItems.map(x => x.id);


Here you're testing against the internal propertysortedNewsListItems and not what's being rendered. This puts you under a high risk of false-negatives.

I tried to fix that but it's a pretty ugly solution in my opinion. I am testing against the wrapper.find("newslist").element.innerHTML. It feels very wrong because I obtained the "correct" string by printing this exact thing and looking through it for possible errors. And this is still not the final string that actually gets rendered by the browser ... Do you have a better way of doing this in mind?

EDIT: We found another way, commit coming soon!

expect(wrapper.findAllComponents(NewsItem).wrappers.map(w => w.text())) .toEqual(['Title a (2)', 'Title b (1)'])

webapp/dojo_news/src/App.spec.js

webapp/dojo_news/src/App.vue

Co-authored-by: Robert Schäfer <[email protected]>

…code

Co-authored-by: Robert Schäfer <[email protected]>

jobegrabber requested review from roschaefer, donatbrzoska and medizinmensch November 5, 2020 12:23

jobegrabber force-pushed the exercise2 branch 3 times, most recently from 091e75f to df5495f Compare November 9, 2020 18:31

jobegrabber force-pushed the exercise2 branch from df5495f to 9153cfa Compare November 9, 2020 21:57

jobegrabber marked this pull request as ready for review November 9, 2020 21:58

jobegrabber requested review from s0557917 and powlaa and removed request for s0557917 November 10, 2020 09:07

donatbrzoska requested a review from noeljns November 10, 2020 18:24

noeljns approved these changes Nov 10, 2020

View reviewed changes

powlaa approved these changes Nov 10, 2020

View reviewed changes

powlaa mentioned this pull request Nov 11, 2020

Feature/exercise2 Systems-Development-and-Frameworks/lichtow#5

Merged

roschaefer requested changes Nov 12, 2020

View reviewed changes

donatbrzoska force-pushed the exercise2 branch from e9104ed to cebc3bf Compare November 14, 2020 23:32

Jonas Grabber added 10 commits November 17, 2020 21:56

Add testing packages

92182c6

Add message on empty news list

ca387ce

Remove archive and unused images

96e1ca7

Rework READMEs

dab8766

Add CI workflow

bfcbd39

Run CI in webapp/dojo_news by default

23d5eb1

Rename workflow to Build, Test, Lint

04b320a

Add badge to README

3ae2089

Add link to build badge

5256deb

Fix headings in README

2820c85

Jonas Grabber and others added 21 commits November 17, 2020 21:57

Add mutation tests to App spec + fixes accordingly

19a70e5

add reverse list items feature with tests

eb68b1d

divide tests into sections

724e0bc

fix naming

85af7d4

isolate App test from other components

de05a34

Add some more tests, remove invalid comments

7f9148c

Add stories and improve accessiblity

26091c8

Simplify news list ordering computed

fcc0b07

Reverse Order Button is now disabled instead of removed upon empty list

155f77b

use jest's toHaveLength function

97e3937

Co-authored-by: Robert Schäfer <[email protected]>

use toBeEmptyDOMElement function instead of directly looking at HTML …

8bccb1d

…code

remove double checks for the same purpose

a2ef2f4

use wrapper function instead of dom element function

2c55898

Co-authored-by: Robert Schäfer <[email protected]>

remove unnecessary brackets

3525458

Co-authored-by: Robert Schäfer <[email protected]>

have two test cases instead of one for mutating the news list

51ba50d

fix remainder of old test case version

b8e9c1a

split list empty message test case into to test cases

ef27f28

rename test cases for a better test result reading experience

599688e

use hard coded string for list empty message in tests

011447a

comparing against "rendered" HTML instead of internal computed variable

6297bcb

Don't compare results with plain (copied) HTML

fa39e58

jobegrabber force-pushed the exercise2 branch from 14c2886 to fa39e58 Compare November 17, 2020 21:00

jobegrabber merged commit de87365 into main Nov 17, 2020

jobegrabber deleted the exercise2 branch November 17, 2020 21:03

jobegrabber pushed a commit that referenced this pull request Dec 6, 2020

docs: add exercise #4

a3f1a84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exercise2 #4

Exercise2 #4

jobegrabber commented Oct 28, 2020

s0557917 commented Nov 9, 2020 •

edited

Loading

jobegrabber commented Nov 9, 2020 •

edited

Loading

s0557917 commented Nov 9, 2020

noeljns Nov 10, 2020

jobegrabber Nov 17, 2020

roschaefer Nov 18, 2020 •

edited

Loading

powlaa left a comment

powlaa Nov 10, 2020

roschaefer Nov 12, 2020

donatbrzoska Nov 15, 2020 •

edited

Loading

roschaefer Nov 15, 2020

donatbrzoska Nov 15, 2020

roschaefer left a comment

roschaefer Nov 12, 2020

donatbrzoska Nov 15, 2020 •

edited

Loading

roschaefer Nov 15, 2020 •

edited

Loading

Exercise2 #4

Exercise2 #4

Conversation

jobegrabber commented Oct 28, 2020

s0557917 commented Nov 9, 2020 • edited Loading

jobegrabber commented Nov 9, 2020 • edited Loading

s0557917 commented Nov 9, 2020

noeljns Nov 10, 2020

Choose a reason for hiding this comment

jobegrabber Nov 17, 2020

Choose a reason for hiding this comment

roschaefer Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

powlaa left a comment

Choose a reason for hiding this comment

powlaa Nov 10, 2020

Choose a reason for hiding this comment

roschaefer Nov 12, 2020

Choose a reason for hiding this comment

donatbrzoska Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

roschaefer Nov 15, 2020

Choose a reason for hiding this comment

donatbrzoska Nov 15, 2020

Choose a reason for hiding this comment

roschaefer left a comment

Choose a reason for hiding this comment

roschaefer Nov 12, 2020

Choose a reason for hiding this comment

donatbrzoska Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

roschaefer Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

s0557917 commented Nov 9, 2020 •

edited

Loading

jobegrabber commented Nov 9, 2020 •

edited

Loading

roschaefer Nov 18, 2020 •

edited

Loading

donatbrzoska Nov 15, 2020 •

edited

Loading

donatbrzoska Nov 15, 2020 •

edited

Loading

roschaefer Nov 15, 2020 •

edited

Loading