udm

Merge lp://qastaging/~mandel/udm/add-tests into lp://qastaging/udm

Proposed by Manuel de la Peña
Status: Merged
Merged at revision: 4
Proposed branch: lp://qastaging/~mandel/udm/add-tests
Merge into: lp://qastaging/udm
Diff against target: 933 lines (+629/-101)
4 files modified
common.go (+158/-0)
common_test.go (+83/-0)
downloader.go (+82/-101)
downloader_test.go (+306/-0)
To merge this branch: bzr merge lp://qastaging/~mandel/udm/add-tests
Reviewer Review Type Date Requested Status
Sergio Schvezov Needs Fixing
Review via email: mp+219200@code.qastaging.launchpad.net

Commit message

Add missing tests.

To post a comment you must log in.
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

41 +type watch interface {

for quick reading can you call this watchInterface or something of the sort? Same for proxy perhaps

43 + Chanel() chan *dbus.Message

Channel?

+ return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)

errors should always be lower case to be able to concatenate better in logs et.al. DBus is fine since it's a proper name, but 'Error' should be 'error'

--
func (down *FileDownload) Throttle() (throttle uint64, err error) {
 reply, err := down.proxy.Call(DOWNLOAD_INTERFACE, "throttle")
 if err != nil {
  return 0, err
 }

 if reply.Type == dbus.TypeError {
  return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)
 }

 if err = readArgs(reply, &throttle); err != nil {
  return 0, err
 }
 return throttle, nil
}

How about

func (...) Blah() (...) {
    return down.getUint64Value("throttle")
}

func (...) getUint64Value(valueName string) (uint64, error) {
 reply, err := down.proxy.Call(DOWNLOAD_INTERFACE, valueName)
 if err != nil {
  return 0, err
 }

 if reply.Type == dbus.TypeError {
  return 0, fmt.Errorf("DBus Error: %", reply.ErrorName)
 }

        var value uint64
 if err = readArgs(reply, &value); err != nil {
  return 0, err
 }
 return value, nil
}

rinse and repeat for progress, totalSize, et.al; similar logic for others

595 + . "gopkg.in/check.v1"

As mentioned in the previous Needs Fixing, you need to use launchpad.net/gocheck

Finally, can you run goimports against the code? It formats internal and external imports nicely.

review: Needs Fixing
lp://qastaging/~mandel/udm/add-tests updated
3. By Manuel de la Peña

Make changes according to the reviews.

4. By Manuel de la Peña

Use gocheck from lp

5. By Manuel de la Peña

Fixed decoding issues.

6. By Manuel de la Peña

Fmt

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches