msc_vlr_tests patches

[Neels Hofmeyr]

On Tue, Feb 13, 2018 at 02:17:17PM +0100, Max wrote:
> > The reason why I wanted that number output only in verbose mode is, I don't
> > want to adjust unrelated *.err output when adding a test in any position that
> > is not the last.
> 
> Curious. When would we want to do that? I assumed tests are independent of
> each other. That's not the case?

The tests were independent, but since they now print their number in the
expected output, they no longer are. To illustrate, some pseudocode:

Before:

  ===== test_no_auten...
  ===== test_no_auten_imei...

Now:

  ===== [1] test_no_auten...
  ===== [2] test_no_auten_imei...

If I now add a test

  ===== [1] test_no_auten...
  ===== [2] test_no_auten_tmsi...  <-- new
  ===== [3] test_no_auten_imei...

leads to "Test failed" because the number changed:

    ===== [1] test_no_auten...
    ===== [2] test_no_auten_tmsi...
  - ===== [2] test_no_auten_imei...
  + ===== [3] test_no_auten_imei...

All subsequent tests' output needed adjustment.

This happened a lot during early development, because I wanted semantic
order instead of only adding in the end, hence I had those indexes output
only when -v was passed so they don't mess up expected output.

> The idea is to make IMSI into actual parameter so it would be easy to
> introduce tests with different edge cases - shorter-than-normal,
> longer-than-normal IMSI etc. It have not materialized (yet?) though due to
> way too many hardcoded hex strings in tests.

I see that idea, but it goes against how these tests were conceived: they
are on purpose passing in raw hex streams of data. It has disadvantages
but also guarantees to see exactly what is being tested in all clarity.

The disadvantage is that you can't parameterize the hex strings easily.
But I on purpose didn't want to incorporate our encoding functions and use
structs. One reason is practical: I can just copy paste PDU data from a
pcap of an actual real test, and from that point on I am testing exactly
what the phone sent. Apart from that, I also generally discourage
parameterizing tests (I do in few cases to spare some heavy code
duplication, but try to minimize or avoid altogether). The reason is: when
everything from the test side is hardcoded and actual duplicated code,
each test is ultimately independent and terminally clear. After all it's
not production code that we need to maintain in the sense of API, their
purpose is to model a rock hard situation that should be easy to step
through if a test fails, without figuring out which parameters were passed
and which code paths were taken.

TTCN3 is more elegant in the sense that it "adds ways to compose the PDUs
more flexibly"-squared. Notably it also employs its own and independent
protocol definitions and also doesn't use our own encoding functions.

An advantage of the msc_vlr_tests is that they are blazingly fast and
independent of external dependencies etc., so they tell you right away
during development what you've changed.

So those are the guidelines the msc_vlr_tests were based on. If I were to
test longer and shorter IMSIs as msc_vlr_test, I would simply add another
test, copy-pasting an existing one and hand editing the IMSI. May sound
crazy, but for testing code my opinion is that this is the actual way to
do it, and it is consciously kind of opposite of API development. Doing
that in ttcn-3 is a good alternative, but parameterizing *all* of the
msc_vlr_tests clearly isn't.

(I already pointed Max at a test that would be convenient to adjust, last
week in person: test_lu_unknown_tmsi() does an IMSI identity request, and
one only needs to change the IMSI in the reply.)

> > 3) you moved gsm_network creation into a separate function, but that function
> > is still called only once. So it's just cosmetic, or was there another purpose?
> > 
> See above.

See where? Was there another purpose or not? AFAICT gsm_network creation
is still and probably will always be called only once from the main()
function.

~N

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20180219/83ec6be0/attachment.bin>