ARM-software / trappy

This repository has moved to https://gitlab.arm.com/tooling/trappy
Apache License 2.0
60 stars 39 forks source link

tests/test_ftrace: Fix test_get_all_freqs_data() #289

Closed valschneider closed 5 years ago

valschneider commented 5 years ago

In Python2, iterating over a dict's keys will always give the same sequence (although the ordering is arbitrary). In Python3 that ordering is still abitrary, but to make things even better it can change from one execution to another.

GenericFTrace.get_all_freqs_data() returns a list who's ordering depends on the keys of the input dictionnary. That in itself would need to be changed to a better design, but for the time being we can fix the test and make Trappy pass reliably by using an OrderedDict.

douglas-raillard-arm commented 5 years ago

That behavior was already there in Python 2 as a counter measure for some attacks, but indeed was disabled by default to avoid breaking brittle code, and is now the default in Python 3: https://docs.python.org/2/using/cmdline.html#cmdoption-r

As for the fix, wouldn't that be a better idea to fix TestFTrace.test_get_all_freqs_data() directly ? The test is currently not correct since get_all_freqs_data() makes no promise about the order of the list.

This should fix the test by turning allfreqs into a dict, which also makes it much clearer:

    def test_get_all_freqs_data(self):
        """Test get_all_freqs_data()"""

        allfreqs = dict(trappy.FTrace().get_all_freqs_data(self.map_label))

        self.assertEqual(allfreqs['A53']["A53_freq_out"].iloc[3], 850)
        self.assertEqual(allfreqs['A53']["A53_freq_in"].iloc[1], 850)
        self.assertEqual(allfreqs['A57']["A57_freq_out"].iloc[2], 1100)
        self.assertTrue("gpu_freq_in" in allfreqs['GPU'].columns)

        # Make sure there are no NaNs in the middle of the array
        self.assertTrue(allfreqs['A57']["A57_freq_in"].notnull().all())

Also it looks like GenericFTrace.plot_allfreqs() is broken, since it allows passing a list of axis in ax parameter, which is supposed to be in the same order as get_all_freqs_data(), which is obviously impossible to get right since the latter is random.

valschneider commented 5 years ago

I didn't realize the output for get_all_freqs_data() was suitable to be directly converted into a dict. It probably should return a dict by default, but that's beyond the point - I just wanted to get the tests passing reliably for now.

valschneider commented 5 years ago

I had forgotten about this, I think it's good to go now :)