WIP slave command behavior assertion system
Needs ReviewPublic

Authored by sitter on Oct 23 2019, 12:44 PM.

Details

Reviewers
dfaure
Summary

This implements a simple expectation system covering slave commands.
At its heart sits the CommandExpectation, which contains the expectations
of a given command. Currently the sole expectation is about signal emission
notably a whitelist of signals that any given command may emit during its
emission. To assist this there's a new Signal enum which is used to
refer to the signals. Signals by default are considered exclusive and
repeatedly emitting the same signal will result in assertion failures.
Multi-emission signals such as data/written/processedsize are of course
excluded from this.
All signal implementation in SlaveBase should call VERIFY_CALL to verify
the validity of the signal call at call-time (notably: can this signal
be called during this Command and may it be called 1 or N times).
After the command finished the pre-existing verifyState function has
been refitted for the new expectation tech.

  • slavebase.h now tries to document expectations WRT signals
  • makes KIO to a Q_NAMESPACE which kinda exposes unintended symbols I guess e.g. staticMetaObject. this does enable us to convert the enum values to strings via QMetaMethod but has no additional purpose beyond that.
  • m_state/States are no more. m_state was solely used to track whether error/finished was called and then assert expectations. this is replaced by m_receivedSignals which tracks any number of signals
  • verifyErrorFinishedNotCalled and verfifyFinishedNotCalled have been merged into verifyCall which is now the one-place-stop to get in-call verification (i.e signals call verifyCall and verifyCall makes sure that signal should even have been called)
  • verifyState has been reworked to be applicable to all expectations

todo:

  • I am not 100% on most of the expectations
  • do we really want Q_NAMESPACE on KIO? we could replace this with enumToString() functions switching over the values manually. seeing as I am lazy I'd rather use the moc generated data though
  • conceivably an empty expectation should mean everything is allowed, currently it's meant to mean nothing is allowed as that is rather more handy to make sure the expectations themselves are in fact correct and complete
  • warning todos
  • stray debugs
Test Plan

casually use all the slaves

Diff Detail

Repository
R241 KIO
Branch
assert-file
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18070
Build 18088: arc lint + arc unit
sitter created this revision.Oct 23 2019, 12:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 23 2019, 12:44 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Oct 23 2019, 12:44 PM

About the list of unexpected signals in forCommand():

It looks that at least http, ftp, and sftp do call connected() as part of various commands. e.g. all do it during get(). Does that make sense?
connected() refers to openConnection() in its documentation, and openConnection() says the slave is operating in connection-oriented mode when called, so if openConnection() puts it into connection-oriented mode then not having had a call to openConnection() means it shouldn't be operating in connection-oriented mode. I have no additional understanding besides what the documentation tells me and so it would seem to me that connected() shouldn't be called anywhere but openConnection(). At the same time the documentation for openConnection() does have the neat qualifier forced * Opens the connection (forced)

So I guess my questions are :

  • what does connection-oriented mean exactly?
  • what exactly is a forced connection open?
  • how is a forced open different from a casual open?
  • should connected() really be called all over the place?
  • if so, does the client software actually need to do something based on the signal or is it simply a case of "it does no harm, so emitting it unconditionally is easier than not"?

Based on our specific requirements here the expectation class needs some rejiggering to differentiate state violations (e.g. openConnection() neither calling connected() nor error()) from unexpected updates (connected() during get()) from useless updates (listEntry() during stat()).