# Update salobj test_salobj_to_either to work with SAL 6

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
1
• Sprint:
TSSW Sprint - Jun 07 - Jun 21
• Team:
Telescope and Site
• Urgent?:
Yes

#### Description

ts_salobj tests/test_salobj_to_either.py fails with ts_sal develop ad169f42da1aa11280d5734b1c0ed4cdc9a09486 (2021-06-07) with:

 > raise base.AckError(msg="Command failed", ackcmd=ackcmd) E lsst.ts.salobj.base.AckError: msg='Command failed', ackcmd=(ackcmd private_seqNum=1377420745, ack=, error=1, result='Commanding not permitted by authList setting') 

Work around this, e.g. by making the Remote name a CSC name.

#### Activity

Hide
Russell Owen added a comment - - edited

I tested the simple fix using old (probably version 5) and new (current develop) ts_sal.

I also improved DefaultingValidator's documentation to explain that defaults deeper than 2 levels are ignored.

Pull request: https://github.com/lsst-ts/ts_salobj/pull/194

Show
Russell Owen added a comment - - edited I tested the simple fix using old (probably version 5) and new (current develop) ts_sal. I also improved DefaultingValidator 's documentation to explain that defaults deeper than 2 levels are ignored. Pull request: https://github.com/lsst-ts/ts_salobj/pull/194
Hide
Wouter van Reeven added a comment - - edited

So, as I tried to explain on GitHub, what I was struggling with is to skip a default value on a level and to add them on the next level like this. Note how devices doesn't have a default. The DefaultingValidator then removes devices and all sub-levels. Perhaps it should be explained that all higher levels need to have a default value?

 properties:  devices:  type: array  items:  type: object  properties:  name:  description: Name of the sensor  type: string  default: EssTemperature4Ch  channels:  description: Number of channels  type: integer  default: 4  type:  description: Type of the device  type: string  enum:  - FTDI  - Serial  default: FTDI  

Show
Wouter van Reeven added a comment - - edited So, as I tried to explain on GitHub, what I was struggling with is to skip a default value on a level and to add them on the next level like this. Note how devices doesn't have a default. The DefaultingValidator then removes devices and all sub-levels. Perhaps it should be explained that all higher levels need to have a default value? properties: devices: type: array items: type: object properties: name: description: Name of the sensor type: string default : EssTemperature4Ch channels: description: Number of channels type: integer default : 4 type: description: Type of the device type: string enum : - FTDI - Serial default : FTDI
Hide
Wouter van Reeven added a comment -

Reviewed on GitHub and a added comment.

Show
Wouter van Reeven added a comment - Reviewed on GitHub and a added comment.
Hide
Russell Owen added a comment - - edited

The new example shows a case where all higher levels do not have a default. There is no default for subdict1 yet it still shows up because the items within it all have default values.

The example doesn't show an array, but my guess is the default for that would have to be at the level of the array itself. I think this gets into the level of "try it and see what works". Which is why I always recommend users have a test_validation.py unit test that checks the schema.

Show
Russell Owen added a comment - - edited The new example shows a case where all higher levels do not have a default. There is no default for subdict1 yet it still shows up because the items within it all have default values. The example doesn't show an array, but my guess is the default for that would have to be at the level of the array itself. I think this gets into the level of "try it and see what works". Which is why I always recommend users have a test_validation.py unit test that checks the schema.
Hide
Wouter van Reeven added a comment -

The empty default was a typo and I removed it in the example. I'll test again to make sure that it doesn't work without a default value on that level.

Show
Wouter van Reeven added a comment - The empty default was a typo and I removed it in the example. I'll test again to make sure that it doesn't work without a default value on that level.
Hide
Russell Owen added a comment -

I was wondering! I commented on the empty default and then looked at it was gone.

Another thought on the default for a list: I think you must have the default at that level because otherwise there is no way to know how many entries you want to have by default. So the default belongs there.

Note that this will become irrelevant if Tiago and Michael's proposal to handle defaults in config_ packages is accepted.

Show
Russell Owen added a comment - I was wondering! I commented on the empty default and then looked at it was gone. Another thought on the default for a list: I think you must have the default at that level because otherwise there is no way to know how many entries you want to have by default. So the default belongs there. Note that this will become irrelevant if Tiago and Michael's proposal to handle defaults in config_ packages is accepted.
Hide
Russell Owen added a comment -

Released as v6.3.8

Show
Russell Owen added a comment - Released as v6.3.8

#### People

Assignee:
Russell Owen
Reporter:
Russell Owen
Reviewers:
Wouter van Reeven
Watchers:
Dave Mills, Michael Reuter, Russell Owen, Tiago Ribeiro, Wouter van Reeven