Replace 'defines correct option sets' tests with validation tests

[waldekmastykarz]

In some commands we have the 'defines correct option sets' test that verifies if the command defines the correct option sets. Thinking of it, it's not the right way about checking validation. Each time we change something about the command's option sets, we need to adjust the tests as well, which means we need to apply each change in two places. It also tightly couples the implementation with the test. A better way about it would be to add tests that verify that command fails validation when incorrect options are passed, irrespective how it's implemented in the command itself.

Before we start implementation, we should list all commands that require changing.

[Jwaegebaert]

+1 on this idea. If I'm reading it correctly, this will require users to write more specific tests to validate their options?

[martinlingstuyl]

It ties it down to the implementation, that's true. On the other hand, one of the reasons we added option sets is to lift the burden of writing the same code again and again. We are sparing the contributor writing x extra lines of code and validation tests.

With optional and dependent option sets coming up, that gain will increase. Will it not be much easier (for the contributors) if we implement the optionsets test logic in a central place that we can call from the separate command spec files? It will save a lot of lines of code and a lot of work.

[milanholemans]

It ties it down to the implementation, that's true. On the other hand, one of the reasons we added option sets is to lift the burden of writing the same code again and again. We are sparing the contributor writing x extra lines of code and validation tests.

With optional and dependent option sets coming up, that gain will increase. Will it not be much easier (for the contributors) if we implement the optionsets test logic in a central place that we can call from the separate command spec files? It will save a lot of lines of code and a lot of work.

Agree. I think the logic to validate how option sets work should not be tested in the command test itself. The defines correct option sets test now prevent the user from 'accidentally' updating the option set, you can argue if that is useful or not. In the latter case, we can just remove them, but I wouldn't really add more tests to test something that is centralized. I think the command tests should be testing only the logic added in the command file and not utils, base classes, ...

[waldekmastykarz]

+1 on this idea. If I'm reading it correctly, this will require users to write more specific tests to validate their options?

Correct @Jwaegebaert

Will it not be much easier (for the contributors) if we implement the optionsets test logic in a central place that we can call from the separate command spec files? It will save a lot of lines of code and a lot of work.

Unfortunately, we can't do that @martinlingstuyl, because tests run in the context of a command, and only the command knows which options belongs to a set. Our tests should be decoupled from the how we implement validation, and should let us verify the correct behavior, even if the implementation changes. If we need to change tests each time we change the command's implementation, then in the long run it will lead to more effort.

[nicodecleyre]

Before we start implementation, we should list all commands that require changing.

There are 170 commands that use option sets, 112 of them (in bold) don't use a specific option sets test:

• aad app add
• aad app get
• aad app remove
• aad app role add
• aad app role list
• aad app role remove
• aad app set
• aad approleassignment add
• aad approleassignment list
• aad approleassignment remove
• aad groupsettingtemplate get
• aad o365group conversation post list
• aad o365group recyclebinitem remove
• aad o365group recyclebinitem restore
• aad o365group teamify
• aad o365group user add
• aad o365group user remove
• aad o365group user set
• aad sp add
• aad sp get
• aad user get
• aad user set
• adaptivecard send
• booking business get
• outlook message list
• outlook message move
• pa app get
• planner bucket add
• planner bucket get
• planner bucket list
• planner bucket remove
• planner bucket set
• planner plan add
• planner plan get
• planner plan list
• planner plan remove
• planner task add
• planner task get
• planner task reference remove
• planner task remove
• pp card clone
• pp card get
• pp card remove
• pp managementapp add
• pp solution get
• pp solution publisher get
• pp solution publisher remove
• pp solution remove
• search externalconnection get
• search externalconnection remove
• spo app deploy
• spo app get
• spo contenttype get
• spo contenttype remove
• spo contenttype set
• spo customaction get
• spo customaction remove
• spo eventreceiver get
• spo eventreceiver remove
• spo field get
• spo field remove
• spo field set
• spo file checkin
• spo file checkout
• spo file get
• spo file remove
• spo file roleassignment add
• spo file roleassignment remove
• spo file roleinheritance break
• spo file roleinheritance reset
• spo file sharinginfo get
• spo file version clear
• spo file version get
• spo file version list
• spo file version remove
• spo file version restore
• spo folder get
• spo group add
• spo group get
• spo group member add
• spo group member list
• spo group member remove
• spo group remove
• spo group set
• spo hubsite connect
• spo hubsite disconnect
• spo hubsite get
• spo list contenttype add
• spo list contenttype default set
• spo list contenttype list
• spo list contenttype remove
• spo list get
• spo list label get
• spo list label set
• spo list remove
• spo list roleassignment add
• spo list roleassignment remove
• spo list roleinheritance break
• spo list roleinheritance reset
• spo list set
• spo list sitescript get
• spo list view add
• spo list view field add
• spo list view field remove
• spo list view field set
• spo list view get
• spo list view list
• spo list view remove
• spo list view set
• spo list webhook add
• spo list webhook get
• spo list webhook list
• spo list webhook remove
• spo list webhook set
• spo listitem add
• spo listitem attachment list
• spo listitem get
• spo listitem isrecord
• spo listitem list
• spo listitem record declare
• spo listitem record undeclare
• spo listitem remove
• spo listitem roleassignment remove
• spo listitem roleinheritance break
• spo listitem roleinheritance reset
• spo listitem set
• spo page clientsidewebpart add
• spo site apppermission add
• spo site apppermission remove
• spo site apppermission set
• spo sitedesign get
• spo term add
• spo term get
• spo term group get
• spo term list
• spo term set add
• spo term set get
• spo term set list
• spo user get
• spo user remove
• spo web roleassignment add
• spo web roleassignment remove
• teams app install
• teams app update
• teams channel add
• teams channel get
• teams channel list
• teams channel member add
• teams channel member list
• teams channel member remove
• teams channel member set
• teams channel remove
• teams channel set
• teams chat get
• teams chat message send
• teams tab get
• teams team archive
• teams team clone
• teams team get
• teams team remove
• teams team unarchive
• teams user app list
• todo list get
• todo list remove
• todo list set
• todo task add
• todo task get
• todo task list
• todo task remove
• todo task set

[waldekmastykarz]

Awesome work! If a command test suite doesn't have an option set test, it means that it properly tests for validation already, and we should be able to remove these commands from the list here.

[nicodecleyre]

Would love to work on this one, can you assign me please?

[martinlingstuyl]

Sure thing @nicodecleyre, good luck!