В 2016 году меня пригласили помочь с разработкой экшн-очков "ORBI", это такие водонепроницаемые очки с несколькими камерами, которые могут стримить 360видео сразу на смартфон, ну а если с ними поплавать, тоже ничего сломаться не должно. (https://www.indiegogo.com/projects/orbi-prime-the-first-360-video-recording-eyewear#/). Собственно моей задачей было написать алгоритм склейки видео потока с четырех камер в одно большое 360* видео, на тот момент задача не очень сложная, но требующая немного специфичных знаний opencv и окружения. Но статья не об этом, потому что теперь это все оберегаемое IP, а про то как мы легальными и не очень средствами языка С++ писали тестовое окружение для используемых классов и соответственно алгоритмов. Да вы скажете, что там такого - сделал гетеры да тестируй себе на здоровье. А если гетера нет или переменная класса спрятана в private секцию и возможность изменить хедер отсутствует. Или вендор либы забыл положить хедеры, и прислал только скан исходников (китайские друзья они такие), а тестировать это надо? Помножив желание написать тесты на утренний кофф и приплюсовав дикий энтузиазм, можно получить очень много ошибок компиляции интересного опыта. Если нельзя, но очень хочется, то можно. Как говорил один мой знакомый лид: "Нет такого кода, который мы не сможем порефакторить, особенно за утренним кофф".
На начальном этапе разработки особо не задумывались над общей архитектурой, собрали прототип из свобдоного софта и какойто матери opencv + hugin + stitchEm, поэтому и тесты были раскиданы по всему проекту, включениями разной степени вредности. Ну главное что они хотя бы были и запускались.
К моменту презентации прототипа в конце 2016 года, один из основных классов девайса, который собирал 4 фрейма в один, был обвешан тестами по самое не могу, точно не помню, но чтото около 50 френдов для тестов. Повторюсь сделано это было не специально, так исторически сложилось, а потом стало общей практикой. Каждая неделя разработки добавляла еще пару тестовых друзей в этот класс, и в какойто момент решили что надо эту порочную практику убирать.
Я сейчас вам его покажу. Вот он, этот коварный тип гражданской наружности!
class OrbiVideo421 : public orbi::intrusive_ptr {
friend struct TestVideoStream;
friend struct TestVideoFrame;
friend struct TestVideoStitcher;
friend struct TestVideoPlayer;
friend struct TestVideoTest;
friend struct TestVideoStreamProcessor;
friend struct TestVideoCapture;
friend struct TestVideoWriter;
friend struct TestVideoProcessor;
friend struct TestVideoTestSuite;
friend struct TestVideoAnalyzer;
friend struct TestVideoConfiguration;
friend struct TestVideoBuffer;
friend struct TestVideoRenderer;
friend struct TestAudioStream;
friend struct TestAudioFrame;
friend struct TestAudioStitcher;
friend struct TestAudioPlayer;
friend struct TestPerformanceAnalyzer;
friend struct TestErrorLogger;
friend struct TestVideoFrameAnalyzer;
friend struct TestVideoSynchronization;
friend struct TestVideoMetadataExtractor;
friend struct TestVideoComparison;
friend struct TestVideoStreamController;
friend struct TestVideoStreamEncoder;
friend struct TestVideoStreamDecoder;
friend struct TestVideoStreamRouter;
friend struct TestVideoEffect;
friend struct TestVideoSegmentation;
friend struct TestVideoStreamRecorder;
friend struct TestVideoCompression;
friend struct TestVideoStreamSplitter;
friend struct TestVideoStreamSwitcher;
friend struct TestVideoStreamMetadataEditor;
friend struct TestVideoStreamAnalyzer;
// ниже еще около 40 френдов для тестов
}
Правильно, долго и дорого
class OrbiMemoryUnit : orbi::vtable_at_top {
friend MemoryTestRead;
friend MemoryTestWrite;
private:
nobject *_object;
uint32_t _reserved_memory;
}
Все было спрятано во внутренние структуры, отдельные друзья тесты использовали их по своему усмотрению. Но тут появились красивые мы, и решили сделать по науке, добавив гетеры к нужным мемберам класса. Конечно народ полез не только добавлять гетеры, но и немножко рефакторить код и имена. На горизонте замаячили пара недель мартышкиной работы, и не факт что она окажется полезной, потому что начинает меняться интерфейс класса в угоду тестам. К тому же нарушается правило "breaking changes" - менять без веской причины устоявшийся API - значит получить себе проблемы в будущем с совместимостью и сайдэффектами, которые фиг отловишь. Когда на ревью прилетает рефактор пополам с измененными именами переменных на 300+ строчек, то смотреть это было очень больно. И в итоге после пары созвонов практику эту прекратили, изменения откатили и сели думать как сделать по другому.
class OrbiMemoryUnit : orbi::vtable_at_top {
private:
nobject *_object;
uint32_t _reserved_memory;
public:
inline nobject *object () const { return _object; }
inline uint32_t reserved_capacity () const { return _reserved_memory; }
}
Плюсы такого подхода очевидны, все в рамках языковой модели, понятно новичкам на проекте. Минусы не так очевидны, а может это и не минусы даже, самый явный это необходимость продумывать нормальный API, что на небольших проектах с ограниченным ресурсом и временем растягивает выкатывание фич и отнимает время у команды - о чем мне и было заявлено ПМом на одном из совещаний. Вообщем правильно - но долго и дорого, поэтому неправильно.
Черная магия макросов, но иногда не компилится
Так мы и жили с этим наследием, пока в одно прекрасное утро на один из классов не наткнулся наш коллега из солнечной Катании, который недавно подключился к этому модулю проекта, решив там чего-то дописать. Он был очень unittest friendly погромистом, поэтому число тестов за утро увеличилось в полтора раза. А уже в обед на почту билд инженеру прилетело поздравление, с тем что число френдов в классе OrbiDeviceBattery превысило 100 штук (это мы потом уже выяснили, опытным путем) и компилятор Keil-C ниасилил это скомпилить, вывалив в лог вот такую ошибку.
C:\ent\orbi\prod\KeilMDK\INCLUDE\keillex\xstring(25) : error C2146: syntax error : missing ';' before identifier 'friend'
C:\ent\orbi\prod\KeilMDK\INCLUDE\keillex\xstring(597) : see reference to class template instantiation 'OrbiDeviceBattery<_E,_Tr,_A>' being compiled
C:\ent\orbi\prod\core\battery\device.cc(655) : error C2838: illegal qualified name in member declaration
Каким боком нешаблонный класс OrbiDeviceBattery стал шаблонным, и почему он задел xstring непонятно. Слова в логе об ошибке имеют мало отношения к самой ошибке, мы просто сломали компилятор. Написал саппорту разработчика компилятора и не получил внятного ответа, а побродив по форуму увидел, что подобные жалобы в кор команду компилятора имеют среднее время закрытия от полугода и больше, если зайдете на форум Keil то можно найти парочку, которые были открыты еще в 2017 и до сих пор статусе "незафикшено". Видимо пять лет полежало, и еще пять полежит. На 655 строке ожидаемо был еще один френд класса.
Поскрипев недолго серыми клеточками, которые не восстанавливаются, нашли вполне себе рабочий хак для тестового окружения. Можно переопределить private/class на public/struct. Вроде бы вот оно "щастье" - пиши тесты сколько душе угодно, безо всяких френдов. Но и тут нас ждал розовый птиц обломинго, то что собирается на clang-е, не обязательно будет работать на другом clang-е. Кеil компилятор радостно сообщил, что мы пытаемся переопределить ключевые слова, что как бы делать не стоит, от слова совсем.
#define private public // illegal
#define class struct // illegal
#include "core/view_direction.h"
#include "core/view_vec2i.h"
#include "core/view_point.h"
А вот gcc/clang такое вполне позволяют (https://onlinegdb.com/pag5bTK2Yv). Но вообще не стоит так делать, все равно что растить Пабликов Морозовых в своем проекте. Это мы от безысходности таким страдали. Делайте нормальные решения в рамках языковой модели, это вернется меньшим техдолгом, поверьте моему опыту.
#include <iostream>
using namespace std;
#define private public
#define class struct
class A { int l; };
int main() {
A a;
cout << a.l;
return 0;
}
Дедушка Паблика Морозова
Но задачка уже не хотела отпускать беспокойный мозг, тем более что стартап получил следующий раунд финансирования и можно было немного расслабиться и разогнув спину, заняться чем то более полезным, кроме написания этих ненавистных тестов да придумывания алгоритмов склейки. О проблеме нарушения прав доступа при работе с шаблонами писал Herb Suttter еще в 2010 году (http://www.gotw.ca/gotw/076.htm), но считал её скорее фичей языка на всякий пожарный.
У него даже появилось отдельное правило по этому случаю:
never subvert the language; for example, never attempt to break encapsulation by copying a class definition and adding a friend declaration, or by providing a local instantiation of a template member function (GotW #76), Herb Sutter
Вот пример от самого Саттера (https://onlinegdb.com/cn7bOdWdn), как можно сломать механизм инкапсуляции плюсов, с одной оговоркой, у класса должна быть свободная шаблонная функция в паблик зоне. Работает все достаточно просто, инстанцирование шаблонной функции класса дает нам доступ к приватным данным класса, потому что по факту она является функцией класса со всеми правами.
class X {
public:
X() : private_(1) { /*...*/ }
template<class T>
void f( const T& t ) { /*...*/ }
int Value() { return private_; }
private:
int private_;
};
namespace {
struct Y {};
}
template<>
void X::f( const Y& ) {
private_ = 2; // evil laughter here
}
int main() {
X x;
cout << x.Value() << endl; // prints 1
x.f( Y() );
cout << x.Value() << endl; // prints 2
}
В принципе на этом можно было и остановиться, потому для 90% случае это покрывает необходимости тестирования, и наши френды становятся просто свободными классами, которые будут параметром для вот такой прокси функции. Минимум изменения функционала, максимум пользы, т.е. минус все тестовые френды из хедера. Если бы не одно НО... не везде мы могли добавить такую шаблонную функцию, чтобы провернуть этот фокус.
Всех убил садовник
Еще немного поэкспериментировав с получением приватных членов класса, стало понятно что ни один из существующих компиляторов не палит получение адреса члена класса, если он будет приведен к другому типу данных, который не является приватным. Если мы попробуем получить адрес приватной переменной, то получим ошибку компилятора, что логично, компилятор то умнее нас и оберегает от выстрелов в разные части тела.
struct A {
private:
char const* x = "private data";
};
int main() {
auto ptr = &A::x;
}
...
main.cpp: In function ‘int main()’:
main.cpp:46:19: error: ‘const char* A::x’ is private within this context
46 | auto ptr = &A::x;
| ^
Но если мы аккуратно попросим компилятор дать возможность стрелять куда хочется адрес этой переменной в качестве параметра шаблона, то все будет хорошо. Мы ведь ничего далее с ним не делаем, так - плюшками балуемся.
struct A {
private:
char const* x = "private data";
};
template <class Stub, typename Stub::type x>
struct private_member_x {
private_member_x() { auto ptr = x; }
};
// так мы просим компилятор не обращать внимания на реальный тип переменной
struct A_x { typedef char const *(A::*type); };
// и теперь компилятор считает, что работает с другим типом (подменным)
template struct private_member_x<A_x, &A::x>;
int main()
{}
// все чисто, никаких ошибок компиляции
Итак, адрес приватной переменной у нас есть, остается совсем немного, чтобы написать обертку для хранения и работы с этими адресами. Например как то так (компилябельно) https://onlinegdb.com/MERIajJcH
// шаблон для класса заглушки, который будет хранить адрес переменной
template <class Stub>
struct member { static typename Stub::type value; };
// статик переменная для общего шаблона
template <class Stub>
typename Stub::type member<Stub>::value;
// подмена типа и получение адреса приватной переменной
// stub уйдет дальше в класс member, чтобы имять статик переменную для этого типа
template <class Stub, typename Stub::type x>
struct private_member {
private_member() { member<Stub>::value = x; } // сохранение адреса переменной
};
// тесткейс
struct PapaPavlica {
private:
char const* papini_dengi = "papini dengi";
};
// подменный тип, чтобы компилятор не пугался нарушением прав доступа
struct A_x { typedef char const *(PapaPavlica::*type); };
// магия, здесь живут драконы
template struct private_member<A_x, &PapaPavlica::papini_dengi>;
int main() {
PapaPavlica papa;
// разворачиваем обратно полученный адрес в переменную
// получаем *(papa).(&PapaPavlica::papini_dengi) = "deneg net"
papa.*member<A_x>::value = "deneg net";
std::cout << papa.*member<A_x>::value << std::endl; // deneg net
}
Этот код мы в прод не пустили, он так и остался уделом тестового окружения, но вписался там прекрасно. Ну и конечно, я не оставлю вас без работающей либы (https://github.com/altamic/privablic), купите при встрече нашему любителю юниттестов из солнечной Катании пива.
Комментарии (31)
lorc
22.09.2023 00:02+28Мне кажется что проблема скорее в дизайне. Я так понял что у вас есть god object нечеловеческих размеров, который делает половину работы всей программы. Соответственно, вы хотите тестировать какие-то отдельные куски работы этого объекта. В то время как правильнее было бы иметь объекты более вменяемых размеров и тестировать только их внешнее поведение, оставляя детали внутренней реализации на совести объекта. В таком случае вам бы пришлось дергать только публичные методы. Я так понимаю, это ближе к сути юнит-тестирования.
А возможно (крамольная мысль) что ООП вам вовсе и не нужен... По крайней мере в том модуле.
aamonster
22.09.2023 00:02+1Примерно та же мысль. Мне всегда казалось, что больше половины пользы от юнит-тестов – в том, что вы вынуждены структурировать программу соответствующим образом.
dalerank Автор
22.09.2023 00:02+3Не только вам кажется, использование юниттестов ведет к изменению API вплоть до breaking changes и декомпозиции тестируемых классов, что конечно лучшим образом сказывается на всем проекте. Да только это не всегда можно сделать, по разным причинам. Хорош не тот продукт, в котором все идеально (идеальных вещей вообще нет), а тот который продается и позволяет разививаться дальше (с)
YegorP
22.09.2023 00:02+5Чёт не понял. Я знаю JS/TS (мой хлеб с маслом) и C#. Достаточно различные языки, в которых тем не менее никакие нормальные тесты в приватную часть тестируемого класса не лезут. Всё мокается/перехватывается на подходе, до того как подменяемая зависимость навсегда "пропадёт" в приватке инстанса. А про чисто внутренний стейт я вообще молчу - это личное дело класса, которое не должно беспокоить даже его тесты. Причём в этих языках подглядеть/поменять содержимое private в рантайме ещё проще, чем в плюсах, но этого не делают.
Aquahawk
22.09.2023 00:02+5Вы с чем-то близким к железу работали? Когда надо в тестах полазить во всякие внутренности буферов? Казалось бы не надо, но практика такова что вы без этого часто вообще невозможно понять что идёт не так и какого чёрта оно ведёт себя так, как ведёт. Потому что документация часто неполна, штатный софт крив и работает правильно при определённом стечении обстоятельств. Да, залезая вовнутрь при помощи пабликов морозовых мы огребаем все минусы этого решения. да оно может отвалиться при обновлении, да, всё что вы скажете против паблика да, но это цена которую приходится заплатить чтобы это чудище работало.
dalerank Автор
22.09.2023 00:02+2Есть особенности работы с железом, например надо перехватить буфер на каком то моменте обработки, писать для этого отдельную логику проблематично, обычно просто дампят Стейт класса и дальше уже по офсетам смотрят что где и как лежит
rsashka
22.09.2023 00:02+4А если гетера нет или переменная класса спрятана в private секцию и возможность изменить хедер отсутствует.
У вас для всех примеров требуются изменения в хедерах. Но если хедеры можно изменить, то гораздо проще для тестовых сборок менять приватные и защищенные методы и свойства на публичные каким нибудь макросом, чем заморачиваться со свистопляской , описанной в статье.
Больше времени останется на продуктивную работу, чем искать и исправлять проблему там, где её можно просто обойти.
dalerank Автор
22.09.2023 00:0290% тесткейсов было покрыто шаблонной функцией, последняя часть это уже бонусом было, скорее академические изыскания чем реальный код
ReadOnlySadUser
22.09.2023 00:02+4Всё ещё не понял, чем не устроил Паблик Морозов? Это же код юнит тестов, там можно любые непотребства и антипаттерны в угоду читаемости и независимости тестов друг от друга.
Конечно, лучше проектировать нормально, но когда возможности есть (код чужой/старый), то Паблик Морозов нормальное решение. Если компилятор нельзя победить с его запретами (что странно, диагностики обычно можно задавитт), то просто можно пройтись sed-ом перед компиляцией тестов.
rsashka
22.09.2023 00:02+5#define private public // illegal
#define class struct // illegalЕсли компилятор нельзя победить с его запретами (что странно, диагностики обычно можно задавитт), то просто можно пройтись sed-ом перед компиляцией тестов.
Можно гораздо проще (сам пользуюсь данным способом уже второй десяток лет без каких либо проблем с компиляторами):
#ifdef UNITTEST #define SCOPE(scope) public #else #define SCOPE(scope) scope #endif class Name { SCOPE(private): ... };
ReadOnlySadUser
22.09.2023 00:02Это модификация заголовочных файлов. С этим и sed справится, нет никого смысла городить такие костыли.
rsashka
22.09.2023 00:02+1Вообще-то это sed самый костыльный костыль (тем более, заголовочные файлы правятся в любом случае). А данный способ является кросспатформенным и работает без каких либо проблем в любых компиляторах и операционных системах.
dalerank Автор
22.09.2023 00:02Победить не удалось на тот момент, каких то ключей диагностики я не нашел, чтобы это отключить. А ждать фикса от кейла... ну про полгода на фикс я написал уже
DancingOnWater
22.09.2023 00:02+2Мой опыт показывает, что в 999 случаев из 1000 необходимость в тестировании внутренних методов класса говорит о неправильной декомпозиции. В очень редких случаях, это действительно нужно, но эти тесты уже что-то среднее между интеграционными и юнит-тестами.
dalerank Автор
22.09.2023 00:02Все так, но причину я написал в "долго и дорого", менеджмент внес свои коррективы
Mingun
22.09.2023 00:02Не понял, а добавить один
friend
класс и все тесты наследовать от него нельзя что ли?
https://onlinegdb.com/p5fXE6QPFdalerank Автор
22.09.2023 00:02Нет, во френдах наследование прав не работает, это четко прописано в стандарте
Mingun
22.09.2023 00:02Как я показал в примере, один
friend
класс дает доступ ко всем полям, а тесты наследуются от него (или просто рядом лежат, тогда класс долженpublic
доступ давать). И не надо каждый тест индивидуально делатьfriend
-ом.Просто если и так при создании каждого нового теста правится исходный класс, то как бы логично было сразу сделать один класс, дающий доступ, а все остальные чтобы юзали его. Минимум усилий по переписыванию, идея на поверхности и вообще непонятно, зачем что-то другое изобретать. Только в качестве упражнения.
dalerank Автор
22.09.2023 00:02Такое тоже пробовали, рядом получался еще одни монструозный интерфейс для доступа к приватным данным. Перешли в итоге на шаблонную прокси функцию
Apoheliy
22.09.2023 00:02+3Не про программирование, но всё же: гетеры - это несколько про другое ( https://ru.wikipedia.org/wiki/Гетера ). Может, лучше геТТеры?
dalerank Автор
22.09.2023 00:02+2О_о, я думал эту маленькую шалость никто не заметит, не находите совпадения между функциями которые возвращают приватные данные и сабжем?
federix
22.09.2023 00:02+7Мне же не одному кажется, что если вы пишите юниты на приватные члены, то в разряд говна переходит не только ваш код, архитектура, но и сами юниттесты?
Какой смысл делать что-то приватным, если при его изменении вы сломаете тесты? Делайте частью интерфейса.
vikarti
22.09.2023 00:02Возможно случай когда в приватных методах есть какая то логика которую стоит тестировать и альтернатива — эта логика будет просто не тестируемой?
(И при этом по каким то причинам @VisibileForTesting или его аналог для конкретного языка либо нельзя либо не срабатывает).
У нас вот сейчас такое бывает. Но Kotlin. Android. И по новым внутренним правилам — тесты вообще пишутся по возможности (=ревью можно пройти без тестов).federix
22.09.2023 00:02Ну то есть в вашем объекте есть логика, которая не влияет на объект и вы вынуждены сломать инкапсуляцию чтобы ее проверить?? Вы скорее хотите проверить функцию или объект который неявно туда входит. Выделите их в namespace details и тестируйте наздоровье.
skovoroad
22.09.2023 00:02+1Если вы уже всё равно взялись за ужасные решения, извращающие саму суть ооп, почему бы в тестах не сделать копию определения тестируемого класса без ключевого слова private (например, кодогенерацией) и в тестах просто не реинтерпретировать указатель/ссылку на объект к этому типу? И вот все поля доступны без магии, и исходный код не страдает
(за такое, конечно, увольняют, но за описанное в статье вообще расстреливают)
dalerank Автор
22.09.2023 00:02Было уже, #define private public и работает без кодогенерации. Ну тут мы возвращаемся к вопросу вам шашечки или ехать? ПМ выбрал ехать, бороться с менеджментом?
skovoroad
22.09.2023 00:02+1Define private public тоже ужасно, но оно ещё и аффектит тестируемый код.
Где нет правильных шашечек, со временем перестаёт ехать. Потому что это не просто так шашечки.
Если менеджмент за программистов выбирает инженерные решения, то зачем нужны программисты?
dalerank Автор
22.09.2023 00:02Ок, мы выбрали переделывать архитектуру проекта, чтобы сделать красиво и правильно, отвлекли людей, зарылись в рефактор и не успели по срокам, сорвали раунд финансирования, остались без денег, люди ушли на другой проект (врядли они останутся с нами за добрые слова и обещания всех денег мира). Шашечки правильные, но увы не едет. Я как программист оцениваю техническую сторону, пм оценивает время и возможности, не думайте что они свой хлеб просто так едят. Может они и не умеют в шаблоны, зато прекрасно умеют в планирование.
Т.е. вы готовы увольнять инженеров, которые понимают тонкости языка и могут написать такое решение? Саттер с вами не согласен, некоторое время этот вопрос присутствовал на его собесах в команду архитектов маек
rsashka
22.09.2023 00:02Вы вдвоем приводите в качестве примеров крайности, но почему то забываете, что их как правило не бывает. Точнее, если они случаются, то существуют очень не долго ровно по описанным вами причинам (до закрытия финансирования).
Но ведь в действительности, как менеджер должен принимать в расчеты несколько вариантов архитектурных решений, так и архитектор должен быть в курсе стратегии развития бизнеса, чтобы учесть это при развитии продукта.
Ваши примеры, это принятие решений на основе компромисса двух ролей. Но это очень сложный и зачастую сложный путь, который может окончится ничем (например, если один из двух упрется рогом).
D7ILeucoH
22.09.2023 00:02//Пословица не имей сто рублей а имей сто friend более не актуальна.
//Таких friend за end в museum.
MountainGoat
Метод "Паблик Морозофф" сам применял в дебаге лет 15 назад. Потом перешли на инъекцию записей friend в пайплайне сборки тестов. С тех пор считаю, что правильная реализация private должна позволять его нарушать. Кто оставил в проде ССЗБ, ворнинг кидать можно, но должно работать - для отладки, для юниттестов тоже.