Fix several tests and remove wrong tests

[patriotyk]

I have removed 3 tests. First two DirectorTargetOversizedUptane and ImageRepoTargetOversizedUptane are incorrect because they expect OversizedTarget errors when target from one repo is good and from other is bad. But the problem here is that we don't download images if Image and Director has differnet hashes or length. Third test ImageRepoBadHwIdUptane is incorrect because it expects BadHardwareId from Image repo but image repo don't include hardware ids in its targets metadata.

[pattivacek]

How are these tests passing now if they are broken?

First two DirectorTargetOversizedUptane and ImageRepoTargetOversizedUptane are incorrect because they expect OversizedTarget errors when target from one repo is good and from other is bad. But the problem here is that we don't download images if Image and Director has differnet hashes or length.

Is it possible to keep the tests but change them such that aktualizr's correct behavior of not even downloading them is accepted by the tests?

Third test ImageRepoBadHwIdUptane is incorrect because it expects BadHardwareId from Image repo but image repo don't include hardware ids in its targets metadata.

Odd, that seems just plain wrong. I see there is a test for a bad hardware ID from the director, so I think we can safely remove that one. Does that mean, though, that the tests can inject a hardware ID into the images metadata? How else did that test work?

[patriotyk]

How are these tests passing now if they are broken?

It is because, aktualizr was returning an default counstructed exception (Uptane::Exception last_exception{"", ""};) and in tests we did the next comparison:

if (error["director"]["update"]["err_msg"].asString() == e.what() ||
        error["director"]["targets"][e.getName()]["err_msg"].asString() == e.what() ||
        error["image_repo"]["update"]["err_msg"].asString() == e.what() ||
        error["image_repo"]["targets"][e.getName()]["err_msg"].asString() == e.what()) {
      return true;
    }

those tests expects exceptions from targets but in that case e.getName() was empty string, and error["image_repo"]["targets"] is just std::map. But here we use operator[] which insterts key value constructed by default constructor, so this if statment is always true if we compare with exception created with two empty strings and test expects error from targets

[patriotyk]

Is it possible to keep the tests but change them such that aktualizr's correct behavior of not even downloading them is accepted by the tests?

No, we should not download image if we se different sizes in director and Image metada.

[patriotyk]

Does that mean, though, that the tests can inject a hardware ID into the images metadata? How else did that test work?

No, the tests never inject a hardware ID to the image metadata. This test worked because of another exception: "Hash verification for snapshot metadata failed" whcih was caused by incorrect checsum calculation which I have fixed in my previouse PR. Also aktualizr returned in that case empty exception, which I have explained in my previouse comment

[pattivacek]

It is because, aktualizr was returning an default counstructed exception (Uptane::Exception last_exception{"", ""};) and in tests we did the next comparison

Wow. Good catch!

No, we should not download image if we se different sizes in director and Image metada.

Do we have unit tests that cover that situation in aktualizr? We should still test that somehow even if the original test here was indeed wrong.

No, the tests never inject a hardware ID to the image metadata. This test worked because of another exception

Good catch there, too. Disappointing how broken this all was. Are you confident that the rest of the tests are doing the right thing?

[patriotyk]

Good catch there, too. Disappointing how broken this all was. Are you confident that the rest of the tests are doing the right thing?

No I just checked the tests which began fail.

[lbonn]

@patrickvacek let's merge this? (I don't have commit rights here)