corecfg: handle unknown keys that are explicitly set #3746

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

zyga commented Aug 16, 2017

When updateKeyValueStream is processing a stream with a key that is not
known but is explicitly listed in new configuration it would needlessly
indicate that the file needs writing.

The test that was checking this was flaky because the writes happened
so quickly we were never given a chance to observe the failure
(usually). Running the test repeatedly on my SSD makes it fail very
frequently tough.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

corecfg: handle unknown keys that are explicitly set
When updateKeyValueStream is processing a stream with a key that is not
known but is explicitly listed in new configuration it would needlessly
indicate that the file needs writing.

The test that was checking this was flaky because the writes happened
so quickly we were never given a chance to observe the failure
(usually). Running the test repeatedly on my SSD makes it fail very
frequently tough.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga requested a review from mvo5 Aug 16, 2017

codecov-io commented Aug 16, 2017

Codecov Report

Merging #3746 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3746      +/-   ##
==========================================
+ Coverage   75.72%   75.84%   +0.11%     
==========================================
  Files         398      399       +1     
  Lines       34235    34530     +295     
==========================================
+ Hits        25926    26190     +264     
- Misses       6459     6477      +18     
- Partials     1850     1863      +13
Impacted Files Coverage Δ
corecfg/utils.go 93.75% <100%> (+0.72%) ⬆️
cmd/snap-repair/runner.go 82.96% <0%> (-3.85%) ⬇️
interfaces/sorting.go 98.71% <0%> (-1.29%) ⬇️
asserts/asserts.go 90.92% <0%> (ø) ⬆️
interfaces/builtin/browser_support.go 76.31% <0%> (ø) ⬆️
interfaces/builtin/optical_drive.go 100% <0%> (ø) ⬆️
interfaces/builtin/pulseaudio.go 37.03% <0%> (ø) ⬆️
interfaces/builtin/udisks2.go 100% <0%> (ø) ⬆️
interfaces/builtin/alsa.go 100% <0%> (ø) ⬆️
asserts/store_asserts.go 98.52% <0%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eac3d5d...1758807. Read the comment docs.

Make tests TestConfigurePiConfigNoChange{,Un}Set more robust
The previous test was racy, i.e. on reasonable quick systems
the check for ModTime() is not precise enough to notice that
there was in fact a write. Modify the test so that the test
file can not be written anymore.
Collaborator

mvo5 commented Aug 17, 2017

Thanks for the PR! I looked into this some more and I think we want a slightly different semantic here. The idea behind "allConfig" is really "supportedConfigOptions", i.e. it contains the map of the things updateKeyValueStream understands. I think we want to error if "newConfig" contains things that are not n "allConfig". I prepare a branch for this now (with some better tests) and will also rename "allConfig" into "supportedConfigKeys".

@zyga zyga closed this Aug 17, 2017

@zyga zyga deleted the zyga:fix/updateKeyValueStream branch Aug 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment