Introduction: Zaid Kesarani

you latest PR received some comments, do you grasp the idea behind those comments?

Yaa Iā€™ve already responded to those comments and solved all the issues which Laurent mentioned and submitted new commit after then itā€™s been a 3 days I didnā€™t received any comments from Laurent regarding new commit looks like he is busy. I received one other comment regarding Build Test and Iā€™ve also responded to that comment.

you have continued with @mic704b but Iā€™m a bit concerned about the following comments

Awaiting a ping pong game of Q&As until it is explained in details by Laurent and the others is unlikely to happen.
@bedwardly-down may you can correct or support my om some statements?

2 Likes

Iā€™m getting the impression that those all show carelessness and rushing a bit without fully understanding how the code flows or even knowing to look for the parts that laurent is alluding to.

@kowalskidev, whatā€™s going on? No need to rush if you donā€™t have to.

1 Like
  • To make a build test for my code, I tried looking into existing build tests present in tests folder to understand how build tests are made, I looked for the tests something similar to my code trying to understand what type of build tests required for my code and as written in documentation we need to make new file for new features so I looked into feature_ShowAllNotes.js and feature_TagList.js and I figured out that build tests are made for the events called by GUI to make sure GUI is working correctly and also make sure settings are updated accordingly.

  • So I made feature_ThemeAutoDetect.js file for my code and made tests for the events I created and new settings variables to make sure they are working properly but after the review it turned out that those tests are not needed so I'm little bit confused what to do now? and I've also looked into successful merged PRs but they are related to notes and folders but my case is totally different that's why I asked Laurent if any specific build tests required for my code.

  • That's my mistake I didn't thought of reducing the code. I simply copied theme settings two times and changed name to preferredDarkTheme & preferredLightTheme further I didn't noticed that options property is repeating three time I should reduce to one. I'll take care of this next time...I learned a good lesson that the code should be clean and well structured means I should always try to avoid writing duplicated codes
  • By referring to these functions did you mean nativeTheme.shouldUseDarkColors function? because this function I found by my self, I imported nativeTheme method from electron, It wasn't there before. Laurent is talking about the wrapper that I used to call nativeTheme.shouldUseDarkColors function
  • shouldUseDarkColors is a boolean function which return true for dark system theme and false for light system theme and I wrapped into systemTheme() function to get clear idea of we are getting values from detecting system native theme as simply reading shouldUseDarkColors didn't gives idea that we are getting values from system native theme.

  • Thank you so much for providing me your valuable suggestions and guidance, I'll take care of each of your points next time and I'll work hard to improve myself in upcoming days.

Yes Sir, I understand your concern. Iā€™ll make sure it wonā€™t happen again.

thx for replying to all point.
I would prefer to be less apologizing etc. but more technical.
Less talking. If I read the answer I want to know what is going on instantly.
Only your first reply is technical but challenging to read as it is a whole paragraph, so I get easily lost between the lines

1 Like

Iā€™ve edited my answers now, I hope I explained what you are looking for.

1 Like

The feature tests are for testing integrated behaviour, where we can simulate actions from the UI and check the response, usually by inspecting the resulting state. However for your PR, nearly all the behaviour is in the UI (in ElectronClient), so not much integration to test.

However unit tests are lower level and are used to test individual functions and classes in ReactNative code. You did make a change to one file there ( Setting.js ), try to figure out how it could tested at take from there.

may the following helps you:
joplin/CONTRIBUTING.md at master Ā· laurent22/joplin Ā· GitHub what gets you to https://github.com/laurent22/joplin/pull/2819#issuecomment-603502230

Besides this there is Topics tagged test

1 Like

Thanks. That's a big help! :slightly_smiling_face:

I reviewed my changes in Settings.js, The changes Iā€™ve made is addition of new variables namely themeAutoDetect, preferredLightTheme, preferredDarkTheme and isNightMode. Now to check this settings are working properly I created feature_ThemeAutoDetect.js and make a test like:

it('should update Night Mode setting', asyncTest(async () => {
		// SETUP: Set Night Mode value to true
		testApp.dispatch({
			type: 'SETTING_UPDATE_ONE',
			key: 'isNightMode',
			value: true,
		});
		await testApp.wait();

		const state = testApp.store().getState();
		expect(state.settings.isNightMode).toBe(true);
	}));

This is how settings are tested right? Now talking about unit tests, When I was making build tests for the first time I found models_Setting.js which I think it deals with all the changes in Setting.js here I tried using Setting.setValue() but it returns an error: Settings not initialized thatā€™s why I created feature_ThemeAutoDetect.js and added the test as shown above. Iā€™m still thinking how it can be tested in other way apart from the tests I made.

@Mentors can you giveus some advice on the answer, that is out of my depths

2 Likes

That code looks clean. Something Iā€™ve noticed from a small handful of PRs youā€™ve submitted is that youā€™ve been creating tests that are either redundant or unnecessary. I agree with Pack that one of the devs should look at this, but upon first glance, this looks better in terms of quality.

2 Likes

I looked for build tests related to other events and settings like Night Mode button namely NOTELIST_VISIBILITY_TOGGLE , SIDEBAR_VISIBILITY_TOGGLE, noteListVisibility etc.. just to name a few but I didn't found the specific build tests for these existing settings and events but as Laurent mentioned in the comment on my PR

that's probably already tested somewhere else in a more generic way. We don't need to check this for every single setting value.

So I think events and settings I created are already tested somewhere else along with the existing ones so probably I don't think we need to add new tests for my new settings and events.

Still, Let's wait for other mentors response.

3 Likes