Project

General

Profile

Bug #10840

Feature #9477: Write regression tests and tests for new features

Automatically test dotfiles persistence

Added by anonym over 3 years ago. Updated about 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
01/03/2016
Due date:
% Done:

100%

Feature Branch:
bugfix/10840-automatically-test-dotfiles-persistence
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

Regression test for #10784.


Related issues

Related to Tails - Bug #10784: Dotfiles persistent feature is broken on Jessie Resolved 12/21/2015

Associated revisions

Revision 0e98e38b (diff)
Added by bertagaz about 3 years ago

Is /lib/live/mount/persistence a symlink to /live/persistence?

This addition tests regressions on bug #10784.

Refs: #10840

Revision ce4cb383 (diff)
Added by bertagaz about 3 years ago

Revert "Is /lib/live/mount/persistence a symlink to /live/persistence?"

This reverts commit 0e98e38bd95438d3c0c4cc22c4a6280a5815f175.

That was not the intent of this ticket.

Refs: #10840

Revision 8307257f (diff)
Added by bertagaz about 3 years ago

Test dotfiles persistence feature.

Refs: #10840

Revision 80b2efe5 (diff)
Added by bertagaz about 3 years ago

Move dotfiles persistence test into its own scenario.

Refs: #10840

Revision bec68ea5 (diff)
Added by bertagaz about 3 years ago

Move dotfile persistence test into its own Gherkin functions.

Refs: #10840

Revision 7a22c56a (diff)
Added by intrigeri about 3 years ago

Test suite: fix Gherkin semantics.

"Given" is for setting up the initial (given) state, while "When" is for
describing the action leading to the expected result.

refs: #10840

Revision 5643e182
Added by intrigeri about 3 years ago

Merge branch 'bugfix/10840-automatically-test-dotfiles-persistence' into stable

Fix-committed: #10840

History

#1 Updated by anonym over 3 years ago

  • Assignee set to kytv

kytv, please set an appropriate milestone/target.

#2 Updated by intrigeri over 3 years ago

  • Related to Bug #10784: Dotfiles persistent feature is broken on Jessie added

#3 Updated by intrigeri over 3 years ago

  • Target version set to Tails_2.3
  • Parent task set to #9476

(Setting metadata tentatively so there's no chance it is forgotten, but please update so that it integrates into your roadmap.)

#4 Updated by bertagaz over 3 years ago

  • Assignee changed from kytv to bertagaz

Taking over this ticket.

#5 Updated by bertagaz over 3 years ago

  • Target version changed from Tails_2.3 to Tails_2.4

#6 Updated by intrigeri over 3 years ago

  • Parent task changed from #9476 to #9477

#7 Updated by bertagaz over 3 years ago

  • Target version changed from Tails_2.4 to Tails_2.5

#8 Updated by bertagaz about 3 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20
  • Feature Branch set to bugfix/10840-automatically-test-dotfiles-persistence

Pushed a branch with a patch to test regression on #10784. Sadly it won't be tested by Jenkins, because the whole feature is tagged fragile because of #10720.

#9 Updated by bertagaz about 3 years ago

  • Assignee deleted (bertagaz)
  • QA Check set to Ready for QA

Well, that code runs fine at home, and is so light that I don't think it's worth waiting more. Someone has to look at it and run it but I'm not sure who. Any takers?

#10 Updated by intrigeri about 3 years ago

  • Assignee set to intrigeri

In theory that would be on anomym's plate, but I'm ready to help.

#11 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 20 to 0
  • QA Check changed from Ready for QA to Dev Needed

There seems to be a misunderstanding.

IMO the regression test we want here is "automatically test dotfiles persistence", and is about testing that the functionality keeps working (integration test), not about checking one specific way in which it could be broken (unit test). In particular, because the specific way it's been broken in the past has little chances to occur again (so the added test suite code is mostly useless, but still needs to be maintained), while the general fragileness of this feature makes it likely to break in other ways in the future (so an integration test would be worthwhile).

I'm open to hear your arguments and discuss this further if you disagree.

#12 Updated by bertagaz about 3 years ago

intrigeri wrote:

There seems to be a misunderstanding.

IMO the regression test we want here is "automatically test dotfiles persistence", and is about testing that the functionality keeps working (integration test), not about checking one specific way in which it could be broken (unit test). In particular, because the specific way it's been broken in the past has little chances to occur again (so the added test suite code is mostly useless, but still needs to be maintained), while the general fragileness of this feature makes it likely to break in other ways in the future (so an integration test would be worthwhile).

I'm open to hear your arguments and discuss this further if you disagree.

I don't. I've been tricked by the vague ticket description, so I've tried to reverse enginering what was implied.

#13 Updated by bertagaz about 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Info Needed

So I've reverted the previous commit, updated the branch to the latest stable, and pushed 8307257 which implements the dotfiles feature test. Works fine at home.

I'm not sure it's the best direction, but I thought adding a whole scenario for it was a bit too much overhead: it would have mean yet another scenario with multiple boots of Tails. So I've hooked it along with other persistence tests, but the place where it was easiest to integrate it is in the usb_upgrade feature, and on this I'm unsure. Re-assigning for input on that. Also is there other areas of the dotfiles we'd need to test, or is it already enough?

#14 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Info Needed to Dev Needed

pushed 8307257 which implements the dotfiles feature test. Works fine at home.

That's a pretty good start! You'll want to spell check "persitent" though.

I'm not sure it's the best direction, but I thought adding a whole scenario for it was a bit too much overhead: it would have mean yet another scenario with multiple boots of Tails. So I've hooked it along with other persistence tests, but the place where it was easiest to integrate it is in the usb_upgrade feature, and on this I'm unsure.

Looks like premature optimization to me. Let's please bother about optimization only once we get something that feels right (including from the Gherkin PoV, which is what matters most), works, and proves to affect performance too badly.

is there other areas of the dotfiles we'd need to test, or is it already enough?

I think that's enough :)

#15 Updated by bertagaz about 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 30
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

That's a pretty good start! You'll want to spell check "persitent" though.

Looks like premature optimization to me. Let's please bother about optimization only once we get something that feels right (including from the Gherkin PoV, which is what matters most), works, and proves to affect performance too badly.

Ok, then I've moved this test into its own scenario (and fixed the typo) with commit 80b2efe5. It takes somewhere between 15 and 20 minutes on my hardware.

#16 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Ok, then I've moved this test into its own scenario (and fixed the typo) with commit 80b2efe5.

OK, that's better!

I just have a few comments on the implementation:

  • Including "is working" in the scenario's name is not consistent with the rest of our test suite, and is useless AFAICT.
  • I see one more "persitence" introduced in that commit.
  • I don't get why you're stuffing the new code into the "I write some files expected to persist" function, while not a single line of code is shared between its previous implementation and the additional one you added in there. Just add a "I write some dotfile expected to persist" When function instead? Same question for "the expected persistent files".

It takes somewhere between 15 and 20 minutes on my hardware.

Just to clarify: when run as part of the complete persistence.feature, i.e. with some of the intermediary state restored from snapshots? Or when run in isolation?

#17 Updated by bertagaz about 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

  • Including "is working" in the scenario's name is not consistent with the rest of our test suite, and is useless AFAICT.

Ack.

  • I see one more "persitence" introduced in that commit.

Erf, apologies, seems my fingers can't get to type it correctly (nor my brain to catch their mistake).

  • I don't get why you're stuffing the new code into the "I write some files expected to persist" function, while not a single line of code is shared between its previous implementation and the additional one you added in there. Just add a "I write some dotfile expected to persist" When function instead? Same question for "the expected persistent files".

Because at the Gherkin level, the difference is so tight (1 word) that I thought to factorize it.

It takes somewhere between 15 and 20 minutes on my hardware.

Just to clarify: when run as part of the complete persistence.feature, i.e. with some of the intermediary state restored from snapshots? Or when run in isolation?

Right, that's without snapshots.

Anyway, pushed commit bec68ea

#18 Updated by intrigeri about 3 years ago

Thanks, much better! Pushed 7a22c56ac795bb7bb5f79e3daba912a00ab8663e on top, and code review passes. Will now run this new test. :)

#19 Updated by intrigeri about 3 years ago

  • % Done changed from 30 to 60

#20 Updated by intrigeri about 3 years ago

https://jenkins.tails.boum.org/job/test_Tails_ISO_bugfix-10720-installer-freezes-on-jenkins/67/ is running the added test on Jenkins. I'm also running it locally.

#21 Updated by intrigeri about 3 years ago

It takes somewhere between 15 and 20 minutes on my hardware.

Just to clarify: when run as part of the complete persistence.feature, i.e. with some of the intermediary state restored from snapshots? Or when run in isolation?

Right, that's without snapshots.

Well, what matters to me, as far as this review is concerned, is the answer to: "how much run time does this scenario add to a full run of our test suite, and is this acceptable or do we need to spend time optimizing it?"

I was curious, so I've tested a good approximation of this, on hardware with (hopefully) enough RAM to avoid flushing the test suite's data to disk, and worst case fast disk I/O:

  • full persistence.feature without this new scenario: 18m25s, 17m37s, 18m10s
  • full persistence.feature including this new scenario: 19min49s, 19m58s, 19m19s

… so we're talking of very little additional time ⇒ I say let's not bother with optimizing it at this point. I bet that there are plenty of lower-hanging fruits in this area, where our time would be better spent, for anyone excited in making our test suite run faster. And in passing: these results mean that the new snapshots system is truly awesome! :)

#22 Updated by bertagaz about 3 years ago

intrigeri wrote:

so we're talking of very little additional time ⇒ I say let's not bother with optimizing it at this point. I bet that there are plenty of lower-hanging fruits in this area, where our time would be better spent, for anyone excited in making our test suite run faster. And in passing: these results mean that the new snapshots system is truly awesome! :)

Yes it is! Thanks for your benchmark, that answers the question.

#23 Updated by intrigeri about 3 years ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (intrigeri)
  • % Done changed from 60 to 100
  • QA Check changed from Ready for QA to Pass

Merged!

#24 Updated by intrigeri about 3 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF