invoice-x / factur-x-ng

Python lib for Factur-X, the e-invoicing standard for France and Germany
Other
34 stars 8 forks source link

Added if condition to check for embedded resources in pdf and raise error if file not found #18

Closed duskybomb closed 6 years ago

duskybomb commented 6 years ago

Figured it out After merge we can close #16

m3nu commented 6 years ago

This looks very complicated. Why is test code in the main function? I'm not sure this is the best solution.

duskybomb commented 6 years ago

I don't know if you will approve this, but for testing file_doesn't_exist case I had to move checking to __new__ so that I can return something and test doesn't fail because of Error we are raising.

m3nu commented 6 years ago

This doesn't look right to me. You can look at Alexis' original repo on how he did the checking.

duskybomb commented 6 years ago

The checking part was easy and straight forward see here, but I had to do this to enable unittest to verify that class FacturX is raising an Error.

m3nu commented 6 years ago

You can't change the main code just to enable tests. I'm sure there is another way.

duskybomb commented 6 years ago

I did a lot of research and couldn't find any other. The reason for the change is, we cannot assert (or do anything) after an error is raised. And since we will be enabling CI in future, we need either the test to pass or remove this check(normal check without changing main code).

m3nu commented 6 years ago

You can look at this SO question to get an idea on how to test for correct Exceptions.

duskybomb commented 6 years ago

Obviously I did and implemented it, but the solution is wrong. It is saying something completely different and doesn't apply here.

duskybomb commented 6 years ago

Check this https://stackoverflow.com/a/31918982

m3nu commented 6 years ago

Maybe choose another issue to work on. I'll tackle this one later. Thanks anyways.

m3nu commented 6 years ago

What's wrong with this solution that seems to work well enough?

    def test_input_error(self):
        with self.assertRaises(TypeError):
            FacturX('non-existant.pdf')
duskybomb commented 6 years ago

Now it seems to work, strange. My bad.

But this doesn't

import mymod

class MyTestCase(unittest.TestCase):
    def test1(self):
        self.assertRaises(SomeCoolException, mymod.myfunc)
duskybomb commented 6 years ago

Yeah so we need to make this change to raise the FileNotFoundError this commit and hence tweak the test a little