[cleanup] much cruft in unittests
Author: sshleifer
Pull request: no
Labels: name: Help wanted

name: cleanup

Anti patterns:
- making a result dict and then using each of it's keys. Why use the dict?
- delete all mentions of `check_loss_output`
- use tuple equality: `self.assertEqual(tensor.shape, (bs, seq_len)` instead of 
```python
self.assertListEqual(list(tensor.size()), [bs, seq_len])
```

This does not need to be done for all test files at once.


fix `templates/testing_xxx ` to reflect the new best practice.

Comments:
@stas00, this might be up your alley! 

will do, thank you!

a large part is done: https://github.com/huggingface/transformers/pull/6196


problem: not all `results` in tests are objects, some are plain dict and can't be called with `.key_name`. So there is a large chunk of tests that haven't been re-written because of that.

So we could add a wrapper to the test utils that will make the code consistent with those tests where `results` is an object.

```
class DictAttr:
    def __init__(self, args):
        for k in args:
            setattr(self, k, args[k])

    def __getitem__(self, item):
         return getattr(self, item)
```
and then in the tests:
```
# import DictAttr and then
data = {
    "loss_1": 1,
    "mems_1": 2,
}        
result = DictAttr(data)
```
now it works either way:
```
print(result["loss_1"]) # 1
print(result.loss_1)    # 1
```

Not sure about the best class name for this, suggestions?

So practically, with this change the test code will look

```
--- a/tests/test_modeling_tf_albert.py
+++ b/tests/test_modeling_tf_albert.py
@@ -136,14 +136,12 @@ class TFAlbertModelTester:

         sequence_output, pooled_output = model(input_ids)

-        result = {
+        result = DictAttr({
             "sequence_output": sequence_output.numpy(),
             "pooled_output": pooled_output.numpy(),
-        }
-        self.parent.assertListEqual(
-            list(result["sequence_output"].shape), [self.batch_size, self.seq_length, self.hidden_size]
-        )
-        self.parent.assertListEqual(list(result["pooled_output"].shape), [self.batch_size, self.hidden_size])
+        })
+        self.parent.assertEqual(result.sequence_output.shape, (self.batch_size, self.seq_length, self.hidden_size))
+        self.parent.assertEqual(result.pooled_output.shape, (self.batch_size, self.hidden_size))
```
plus an extra import of whatever the final class name will be.

1) can we just never create a `result` dict? It just creates unneeded indirection.
2) If we need to create a results dict, cant we just do key lookup with `[key]`
3) checkout `collections.UserDict`


> * can we just never create a `result` dict? It just creates unneeded indirection.

From looking at the existing tests, it sort of mimics the returns objects, but doesn't have the accessors for the keys.

So I'm not quite sure what you propose. A short code sample is usually most demonstrative.
 
> * If we need to create a results dict, cant we just do key lookup with `[key]`

I lost you, unless I am misunderstanding what you're suggesting, isn't the big part of this "issue" - replacing `[key]` with `.key`. otherwise nothing else needs to be done and this ticket can be closed - except now it's a mish-mash of results.key (most pt tests) and results["key"] (most tf tests)

> * checkout `collections.UserDict`

I checked  - it doesn't provide `[key]` and `.key` functionality.

ignore #1, I was confused.
when I made this issue, sylvain hadn't merged #6155 , so I guess what remains of the issue is
Sorry for the miscommunication!


Your #6196 will completely close the issue.

So I just need to complete: "delete all mentions of check_loss_output" then - there is one remaining test there.
edit: now done

I plan to do part 2 PR to make the rest of the tests consistent with this change, but I have to wait for this to be merged as it impacts too many files to proceed easily.