Add unit tests for /pkg/rtc #2163
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2163 +/- ##
==========================================
- Coverage 60.31% 59.88% -0.44%
==========================================
Files 399 398 -1
Lines 40606 40671 +65
==========================================
- Hits 24490 24354 -136
- Misses 15152 15358 +206
+ Partials 964 959 -5
Continue to review full report at Codecov.
|
| rtc, err := OpenRTC() | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
There was a problem hiding this comment.
not strictly needed but it is common to just put a
defer rtc.Close()
at a place like this.
There was a problem hiding this comment.
I wasn't sure if the defer would have run on the end of t.Run() or the end of the loop where all the t.Run() are executed in so I went for the safe way and did a rtc.Close() a few lines further down where I would be sure I don't need the rtc anymore.
There was a problem hiding this comment.
ah, ok. I'd suggest putting a comment in about this, so that in three years someone else is as confused as I am :-)
pkg/rtc/rtc_linux_test.go
Outdated
| } | ||
| } | ||
| rtc.Close() | ||
| err = os.Remove(devs[0]) |
There was a problem hiding this comment.
A common pattern is
if err := os.Remove(...); err != nil {
}
There was a problem hiding this comment.
I saw that comment a few times now and I wondered what the reason behind that pattern is. Don't get me wrong, I know it works but why deviate from a uniform pattern only in a few random occasions? Maybe you know something I don't so would be good to know the reason for that pattern.
There was a problem hiding this comment.
The reason for the pattern is to hold the err value for that one particular test. The err variable is captured in that one if err := block. It avoids creating a variable outside the scope of that one particular function call, which can be useful for avoiding accidents (which have happened in practice) either caused by assigning to an err value that already existed; or by creating an err value that causes trouble later. It's a very common pattern, and it's a bit jarring not to see it used, as on lines 43 and 52; if it is ok it would be nice to use it. There is a writeup from the early go days about why to use it but ... I can't find it!
There was a problem hiding this comment.
Ah that makes sense! I'll change it up then, thanks for the explanation.
| t.Error(err) | ||
| } | ||
| devs = []string{f.Name()} | ||
| f.Close() |
There was a problem hiding this comment.
it's common to have a
defer os.Remove(f.Name())
so we don't litter /tmp with these files.
This is a failing of Unix and its share /tmp, but it's all we have ...
There was a problem hiding this comment.
Same here, instead of running defer I manually called os.Remove on it further down in the form of os.Remove(devs[0]) as the devs array only holds a single string that is f.Name()
There was a problem hiding this comment.
For sure this works here. Using defer is just good practice. For this code it does not matter.
| rtc, err := OpenRTC() | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
There was a problem hiding this comment.
ah, ok. I'd suggest putting a comment in about this, so that in three years someone else is as confused as I am :-)
pkg/rtc/rtc_linux_test.go
Outdated
| } | ||
| } | ||
| rtc.Close() | ||
| err = os.Remove(devs[0]) |
There was a problem hiding this comment.
The reason for the pattern is to hold the err value for that one particular test. The err variable is captured in that one if err := block. It avoids creating a variable outside the scope of that one particular function call, which can be useful for avoiding accidents (which have happened in practice) either caused by assigning to an err value that already existed; or by creating an err value that causes trouble later. It's a very common pattern, and it's a bit jarring not to see it used, as on lines 43 and 52; if it is ok it would be nice to use it. There is a writeup from the early go days about why to use it but ... I can't find it!
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Some permission tests that work locally are failing in the CI for unknown reasons. Those are now disabled there. Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
|
Close in favor of #2184 because of CI issues. |
No description provided.