Tame a few tests that relied on `pairs` being somewhat deterministic (#6371)

* Mangle stupid defaults test so that it compares tables, and not a non-deterministic string representation of one.

It's still extremely dumb and annoying to update. (i.e., feel free to kill it with fire in a subsequent PR, I think everybody would cheer).

* Rewrite DepGraph to be deterministic

i.e., fully array based, no more hashes, which means no more pairs randomly re-ordering stuff.

Insertion order is now preserved.

Pretty sure a couple of bugs have been fixed and/or added along the way
;p.

* Resync frontend/apps/filemanager/lib/md.lua w/ upstream

And use orderedPairs in the attribute parsing code, just to make that stupid test happy.
pull/6364/head
NiLuJe 4 years ago committed by GitHub
parent 70f89c4df1
commit 4e5def4282
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -1,4 +1,5 @@
-- From https://github.com/bakpakin/luamd revision 5e8fa39173afecd73913d27e0f0e63649b01fb5b -- From https://github.com/bakpakin/luamd revision 388ce799d93e899e4d673cdc6d522f12310822bd
local FFIUtil = require("ffi/util")
--[[ --[[
Copyright (c) 2016 Calvin Rose <calsrose@gmail.com> Copyright (c) 2016 Calvin Rose <calsrose@gmail.com>
@ -63,7 +64,7 @@ end
-- Line Level Operations -- Line Level Operations
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
local lineDelimiters = {'`', '__', '**', '_', '*'} local lineDelimiters = {'`', '__', '**', '_', '*', '~~'}
local function findDelim(str, start, max) local function findDelim(str, start, max)
local delim = nil local delim = nil
local min = 1/0 local min = 1/0
@ -111,7 +112,7 @@ local function linkEscape(str, t)
if nomatches then externalLinkEscape(str, t) end if nomatches then externalLinkEscape(str, t) end
end end
local lineDeimiterNames = {['`'] = 'code', ['__'] = 'strong', ['**'] = 'strong', ['_'] = 'em', ['*'] = 'em' } local lineDeimiterNames = {['`'] = 'code', ['__'] = 'strong', ['**'] = 'strong', ['_'] = 'em', ['*'] = 'em', ['~~'] = 'strike' }
local function lineRead(str, start, finish) local function lineRead(str, start, finish)
start, finish = start or 1, finish or #str start, finish = start or 1, finish or #str
local searchIndex = start local searchIndex = start
@ -419,7 +420,8 @@ end
local function renderAttributes(attributes) local function renderAttributes(attributes)
local accum = {} local accum = {}
for k, v in pairs(attributes) do -- KOReader: oderedPairs instead of pairs
for k, v in FFIUtil.orderedPairs(attributes) do
accum[#accum + 1] = format("%s=\"%s\"", k, v) accum[#accum + 1] = format("%s=\"%s\"", k, v)
end end
return concat(accum, ' ') return concat(accum, ' ')

@ -11,6 +11,8 @@ Example:
-- The return value of dg:serialize() will be: -- The return value of dg:serialize() will be:
-- {'a2', 'c1', 'b1', 'a1'} -- {'a2', 'c1', 'b1', 'a1'}
NOTE: Insertion order is preserved, duplicates are automatically prevented (both as main nodes and as deps).
]] ]]
local DepGraph = {} local DepGraph = {}
@ -23,98 +25,218 @@ function DepGraph:new(new_o)
return o return o
end end
-- Check if node exists, and is active
function DepGraph:checkNode(id)
for _, n in ipairs(self.nodes) do
if n.key == id and not n.disabled then
return true
end
end
return false
end
-- Returns a (node, node_index) tuple
function DepGraph:getNode(id)
for i, n in ipairs(self.nodes) do
if n.key == id then
return n, i
end
end
return nil, nil
end
-- Like getNode, but only for active nodes:
-- if node is nil but index is set, node is disabled
function DepGraph:getActiveNode(id)
local node, index = self:getNode(id)
if node and node.disabled then
return nil, index
end
return node, index
end
-- Add a node, with an optional list of dependencies
-- If dependencies don't exist as proper nodes yet, they'll be created, in order.
-- If node already exists, the new list of dependencies is *appended* to the existing one, without duplicates.
function DepGraph:addNode(node_key, deps) function DepGraph:addNode(node_key, deps)
if not self.nodes[node_key] then -- Find main node if it already exists
self.nodes[node_key] = {} local node = self:getNode(node_key)
if node then
-- If it exists, but was disabled, re-enable it
if node.disabled then
node.disabled = nil
end
else
-- If it doesn't exist at all, create it
node = { key = node_key }
table.insert(self.nodes, node)
end
-- No dependencies? We're done!
if not deps then
return
end end
if not deps then return end
local node_deps = {} -- Create dep nodes if they don't already exist
local node_deps = node.deps or {}
for _, dep_node_key in ipairs(deps) do for _, dep_node_key in ipairs(deps) do
if not self.nodes[dep_node_key] then local dep_node = self:getNode(dep_node_key)
self.nodes[dep_node_key] = {}
if dep_node then
-- If it exists, but was disabled, re-enable it
if dep_node.disabled then
dep_node.disabled = nil
end end
else
-- Create dep node itself if need be
dep_node = { key = dep_node_key }
table.insert(self.nodes, dep_node)
end
-- Update deps array the long way 'round, and prevent duplicates, in case deps was funky as hell.
local exists = false
for _, k in ipairs(node_deps) do
if k == dep_node_key then
exists = true
break
end
end
if not exists then
table.insert(node_deps, dep_node_key) table.insert(node_deps, dep_node_key)
end end
self.nodes[node_key].deps = node_deps end
-- Update main node with its updated deps
node.deps = node_deps
end end
-- Attempt to remove a node, as well as all traces of it from other nodes' deps
-- If node has deps, it's kept, but marked as disabled, c.f., lenghty comment below.
function DepGraph:removeNode(node_key) function DepGraph:removeNode(node_key)
-- We should not remove it from self.nodes if it has -- We shouldn't remove a node if it has dependencies (as these may have been added via addNodeDep
-- a .deps array (it is the other nodes, that had this -- (as opposed to the optional deps list passed to addNode), like what InputContainer does with overrides,
-- one in their override=, that have added themselves in -- overrides originating from completely *different* nodes,
-- this node's .deps). We don't want to lose these -- meaning those other nodes basically add themselves to another's deps).
-- dependencies if we later re-addNode this node. -- We don't want to lose the non-native dependency on these other nodes in case we later re-addNode this one
local node = self.nodes[node_key] -- with its stock dependency list.
local node, index = self:getNode(node_key)
if node then if node then
if not node.deps or #node.deps == 0 then if not node.deps or #node.deps == 0 then
self.nodes[node_key] = nil -- No dependencies, can be wiped safely
table.remove(self.nodes, index)
else
-- Can't remove it, just flag it as disabled instead
node.disabled = true
end end
end end
-- But we should remove it from the .deps of other nodes. -- On the other hand, we definitely should remove it from the deps of every *other* node.
for curr_node_key, curr_node in pairs(self.nodes) do for _, curr_node in ipairs(self.nodes) do
if curr_node.deps then -- Is not the to be removed node, and has deps
local remove_idx if curr_node.key ~= node_key and curr_node.deps then
for idx, dep_node_key in ipairs(self.nodes) do -- Walk that node's deps to check if it depends on us
for idx, dep_node_key in ipairs(curr_node.deps) do
-- If it did, wipe ourselves from there
if dep_node_key == node_key then if dep_node_key == node_key then
remove_idx = idx table.remove(curr_node.deps, idx)
break break
end end
end end
if remove_idx then table.remove(curr_node.deps, remove_idx) end
end end
end end
end end
function DepGraph:checkNode(id) -- Add a single dep_node_key to node_key's deps
if self.nodes[id] then function DepGraph:addNodeDep(node_key, dep_node_key)
return true -- Check the main node
local node = self:getNode(node_key)
if node then
-- If it exists, but was disabled, re-enable it
if node.disabled then
node.disabled = nil
end
else else
return false -- If it doesn't exist at all, create it
node = { key = node_key }
table.insert(self.nodes, node)
end end
-- Then check the dep node
local dep_node = self:getNode(dep_node_key)
if dep_node then
-- If it exists, but was disabled, re-enable it
if dep_node.disabled then
dep_node.disabled = nil
end
else
-- Create dep node itself if need be
dep_node = { key = dep_node_key }
table.insert(self.nodes, dep_node)
end end
function DepGraph:addNodeDep(node_key, dep_node_key) -- If main node currently doesn't have deps, start with an empty array
local node = self.nodes[node_key] if not node.deps then
if not node then node.deps = {}
node = {} end
self.nodes[node_key] = node
-- Prevent duplicate deps
local exists = false
for _, k in ipairs(node.deps) do
if k == dep_node_key then
exists = true
break
end
end end
if not node.deps then node.deps = {} end if not exists then
table.insert(node.deps, dep_node_key) table.insert(node.deps, dep_node_key)
end end
end
-- Remove a single dep_node_key from node_key's deps
function DepGraph:removeNodeDep(node_key, dep_node_key) function DepGraph:removeNodeDep(node_key, dep_node_key)
local node = self.nodes[node_key] local node = self:getNode(node_key)
if not node.deps then node.deps = {} end if node.deps then
for i, dep_key in ipairs(node.deps) do for idx, dep_key in ipairs(node.deps) do
if dep_key == dep_node_key then if dep_key == dep_node_key then
self.nodes[node_key]["deps"][i] = nil table.remove(node.deps, idx)
break
end
end end
end end
end end
-- Return a list (array) of node keys, ordered by insertion order and dependency.
-- Dependencies come first (and are also ordered by insertion order themselves).
function DepGraph:serialize() function DepGraph:serialize()
local visited = {} local visited = {}
local ordered_nodes = {} local ordered_nodes = {}
for node_key,_ in pairs(self.nodes) do
for _, n in ipairs(self.nodes) do
local node_key = n.key
if not visited[node_key] then if not visited[node_key] then
local queue = { node_key } local queue = { node_key }
while #queue > 0 do while #queue > 0 do
local pos = #queue local pos = #queue
local curr_node_key = queue[pos] local curr_node_key = queue[pos]
local curr_node = self.nodes[curr_node_key] local curr_node = self:getActiveNode(curr_node_key)
local all_deps_visited = true local all_deps_visited = true
if curr_node.deps then if curr_node and curr_node.deps then
for _, dep_node_key in ipairs(curr_node.deps) do for _, dep_node_key in ipairs(curr_node.deps) do
if not visited[dep_node_key] then if not visited[dep_node_key] then
-- only insert to queue for later process if node -- Only insert to queue for later process if node has dependencies
-- has dependencies local dep_node = self:getActiveNode(dep_node_key)
if self.nodes[dep_node_key].deps then -- Only if it was active!
if dep_node then
if dep_node.deps then
table.insert(queue, dep_node_key) table.insert(queue, dep_node_key)
else else
table.insert(ordered_nodes, dep_node_key) table.insert(ordered_nodes, dep_node_key)
end end
end
visited[dep_node_key] = true visited[dep_node_key] = true
all_deps_visited = false all_deps_visited = false
break break
@ -124,11 +246,14 @@ function DepGraph:serialize()
if all_deps_visited then if all_deps_visited then
visited[curr_node_key] = true visited[curr_node_key] = true
table.remove(queue, pos) table.remove(queue, pos)
-- Only if it was active!
if curr_node then
table.insert(ordered_nodes, curr_node_key) table.insert(ordered_nodes, curr_node_key)
end end
end end
end end
end end
end
return ordered_nodes return ordered_nodes
end end

@ -28,32 +28,11 @@ describe("defaults module", function()
assert.is_same("DTAP_ZONE_BACKWARD", Defaults.defaults_name[85]) assert.is_same("DTAP_ZONE_BACKWARD", Defaults.defaults_name[85])
assert.is_same("DCREREADER_CONFIG_WORD_SPACING_LARGE", Defaults.defaults_name[50]) assert.is_same("DCREREADER_CONFIG_WORD_SPACING_LARGE", Defaults.defaults_name[50])
assert.is_same("DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE", Defaults.defaults_name[20]) assert.is_same("DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE", Defaults.defaults_name[20])
local fd = io.open(persistent_filename, "r") dofile(persistent_filename)
assert.Equals( assert.is_same(DCREREADER_CONFIG_WORD_SPACING_LARGE, { [1] = 100, [2] = 90 })
[[-- For configuration changes that persists between updates assert.is_same(DTAP_ZONE_BACKWARD, { ["y"] = 0, ["x"] = 0, ["h"] = 1, ["w"] = 0.25 })
DCREREADER_CONFIG_WORD_SPACING_LARGE = { assert.is_same(DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE, { [1] = 50, [2] = 50 })
[1] = 100, assert.is_same(DDOUBLE_TAP_ZONE_PREV_CHAPTER, { ["y"] = 0, ["x"] = 0, ["h"] = 0.25, ["w"] = 0.25 })
[2] = 90
}
DTAP_ZONE_BACKWARD = {
["y"] = 0,
["x"] = 0,
["h"] = 1,
["w"] = 0.25
}
DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE = {
[1] = 50,
[2] = 50
}
DDOUBLE_TAP_ZONE_PREV_CHAPTER = {
["y"] = 0,
["x"] = 0,
["h"] = 0.25,
["w"] = 0.25
}
]],
fd:read("*a"))
fd:close()
-- in persistent -- in persistent
Defaults:init() Defaults:init()
@ -72,32 +51,21 @@ DDOUBLE_TAP_ZONE_PREV_CHAPTER = {
w = 20.75 w = 20.75
} }
Defaults:saveSettings() Defaults:saveSettings()
fd = io.open(persistent_filename) dofile(persistent_filename)
assert.Equals( assert.is_same(DCREREADER_CONFIG_WORD_SPACING_LARGE, { [2] = 90, [1] = 100 })
[[-- For configuration changes that persists between updates assert.is_same(DDOUBLE_TAP_ZONE_PREV_CHAPTER, {
DCREREADER_CONFIG_WORD_SPACING_LARGE = {
[2] = 90,
[1] = 100
}
DDOUBLE_TAP_ZONE_PREV_CHAPTER = {
["y"] = 0, ["y"] = 0,
["x"] = 0, ["x"] = 0,
["h"] = 0.25, ["h"] = 0.25,
["w"] = 0.75 ["w"] = 0.75
} })
DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE = { assert.is_same(DCREREADER_CONFIG_H_MARGIN_SIZES_XXX_LARGE, { [2] = 50, [1] = 50 })
[2] = 50, assert.is_same(DTAP_ZONE_BACKWARD, {
[1] = 50
}
DTAP_ZONE_BACKWARD = {
["y"] = 10, ["y"] = 10,
["x"] = 10.125, ["x"] = 10.125,
["h"] = 20.25, ["h"] = 20.25,
["w"] = 20.75 ["w"] = 20.75
} })
]],
fd:read("*a"))
fd:close()
os.remove(persistent_filename) os.remove(persistent_filename)
end) end)
@ -120,19 +88,14 @@ DHINTCOUNT = 2
Defaults.changed[57] = true Defaults.changed[57] = true
Defaults.defaults_value[57] = 1 Defaults.defaults_value[57] = 1
Defaults:saveSettings() Defaults:saveSettings()
fd = io.open(persistent_filename) dofile(persistent_filename)
assert.Equals( assert.Equals(DCREREADER_VIEW_MODE, "page")
[[-- For configuration changes that persists between updates assert.is_same(DCREREADER_CONFIG_H_MARGIN_SIZES_LARGE, {
DCREREADER_VIEW_MODE = "page"
DCREREADER_CONFIG_H_MARGIN_SIZES_LARGE = {
[2] = 15, [2] = 15,
[1] = 15 [1] = 15
} })
DGLOBAL_CACHE_FREE_PROPORTION = 1 assert.Equals(DGLOBAL_CACHE_FREE_PROPORTION, 1)
DHINTCOUNT = 2 assert.Equals(DHINTCOUNT, 2)
]],
fd:read("*a"))
fd:close()
os.remove(persistent_filename) os.remove(persistent_filename)
end) end)
end) end)

@ -29,17 +29,17 @@ describe("DepGraph module", function()
dg:addNode('paging_pan_release', {}) dg:addNode('paging_pan_release', {})
assert.are.same({ assert.are.same({
'readerfooter_tap', 'readerfooter_tap',
'readerfooter_hold',
'readermenu_tap', 'readermenu_tap',
'tap_backward', 'tap_backward',
'readerhighlight_hold_pan',
'paging_pan_release',
'readerfooter_hold',
'readerhighlight_hold',
'paging_pan',
'paging_swipe',
'tap_forward', 'tap_forward',
'readerhighlight_tap', 'readerhighlight_tap',
'readerhighlight_hold',
'readerhighlight_hold_release', 'readerhighlight_hold_release',
'readerhighlight_hold_pan',
'paging_swipe',
'paging_pan',
'paging_pan_release',
}, dg:serialize()) }, dg:serialize())
end) end)
@ -56,14 +56,14 @@ describe("DepGraph module", function()
dg:addNode('readermenu_tap', {'readerfooter_tap'}) dg:addNode('readermenu_tap', {'readerfooter_tap'})
assert.are.same({ assert.are.same({
'readerfooter_tap', 'readerfooter_tap',
'readermenu_tap', 'readerfooter_hold',
'readerhighlight_tap', 'readerhighlight_tap',
'readermenu_tap',
'tap_backward', 'tap_backward',
'readerhighlight_hold_pan',
'readerfooter_hold',
'readerhighlight_hold',
'tap_forward', 'tap_forward',
'readerhighlight_hold',
'readerhighlight_hold_release', 'readerhighlight_hold_release',
'readerhighlight_hold_pan',
}, dg:serialize()) }, dg:serialize())
end) end)
@ -85,17 +85,17 @@ describe("DepGraph module", function()
dg:addNode('paging_pan_release', {}) dg:addNode('paging_pan_release', {})
assert.are.same({ assert.are.same({
'readerfooter_tap', 'readerfooter_tap',
'readerfooter_hold',
'readermenu_tap', 'readermenu_tap',
'tap_backward', 'tap_backward',
'readerhighlight_hold_pan',
'paging_pan_release',
'readerfooter_hold',
'readerhighlight_hold',
'paging_pan',
'paging_swipe',
'tap_forward', 'tap_forward',
'readerhighlight_tap', 'readerhighlight_tap',
'readerhighlight_hold',
'readerhighlight_hold_release', 'readerhighlight_hold_release',
'readerhighlight_hold_pan',
'paging_swipe',
'paging_pan',
'paging_pan_release',
}, dg:serialize()) }, dg:serialize())
end) end)
@ -132,9 +132,65 @@ describe("DepGraph module", function()
"tap_backward", "tap_backward",
"tap_forward", "tap_forward",
}, dg:serialize()) }, dg:serialize())
assert.is_true(type(dg.nodes["tap_forward"].deps) == "table") local tapFwdNode = dg:getNode("tap_forward")
assert.is_true(#dg.nodes["tap_forward"].deps > 0) assert.is_true(type(tapFwdNode.deps) == "table")
assert.is_true(type(dg.nodes["tap_backward"].deps) == "table") assert.is_true(#tapFwdNode.deps > 0)
assert.is_true(#dg.nodes["tap_backward"].deps > 0) local tapBwdNode = dg:getNode("tap_backward")
assert.is_true(type(tapBwdNode.deps) == "table")
assert.is_true(#tapBwdNode.deps > 0)
end)
it("should not serialize removed/disabled nodes", function()
local dg = DepGraph:new{}
dg:addNode('foo')
dg:addNode('bar')
dg:addNode('baz',
{'foo', 'bar', 'bam'})
dg:addNode('feh')
dg:removeNode('baz')
dg:removeNode('bar')
dg:addNode('blah', {'bla', 'h', 'bamf'})
dg:removeNode('bamf')
assert.are.same({
'foo',
'bam',
'feh',
'bla',
'h',
'blah',
}, dg:serialize())
-- Check that bamf was removed from blah's deps
assert.are.same({
'bla',
'h',
}, dg:getNode('blah').deps)
-- Check that baz is re-enabled w/ its full deps (minus bar, that we removed earlier) if re-Added as a dep
dg:addNode('whee', {'baz'})
assert.are.same({
'foo',
'bam',
}, dg:getNode('baz').deps)
assert.are.same({
'foo',
'bam',
'baz',
'feh',
'bla',
'h',
'blah',
'whee',
}, dg:serialize())
-- Check that re-adding an existing node with new deps properly *appends* to its existing deps
dg:addNode('baz', {'wham', 'bang'})
assert.are.same({
'foo',
'bam',
'wham',
'bang',
}, dg:getNode('baz').deps)
end) end)
end) end)

@ -1,10 +1,8 @@
describe("MenuSorter module", function() describe("MenuSorter module", function()
local MenuSorter local MenuSorter
local equals
setup(function() setup(function()
require("commonrequire") require("commonrequire")
MenuSorter = require("ui/menusorter") MenuSorter = require("ui/menusorter")
equals = require("util").tableEquals
end) end)
it("should put menu items in the defined order", function() it("should put menu items in the defined order", function()
@ -153,7 +151,7 @@ describe("MenuSorter module", function()
["id"] = "KOMenu:menu_buttons" ["id"] = "KOMenu:menu_buttons"
} }
assert.is_true( equals(result_menu, test_menu) ) assert.is_same(result_menu, test_menu)
end) end)
it("should display submenu of orphaned submenu", function() it("should display submenu of orphaned submenu", function()
local menu_items = { local menu_items = {
@ -178,6 +176,8 @@ describe("MenuSorter module", function()
} }
local test_menu = MenuSorter:sort(menu_items, order) local test_menu = MenuSorter:sort(menu_items, order)
--- @fixme: Currently broken because pairs (c.f., https://github.com/koreader/koreader/pull/6371#issuecomment-657251137)
--print(require("dump")(test_menu))
-- all four should be in the first menu -- all four should be in the first menu
assert.is_true(#test_menu[1] == 4) assert.is_true(#test_menu[1] == 4)

@ -145,11 +145,11 @@ describe("InputContainer widget", function()
assert.is.same('readerfooter_tap', ic._ordered_touch_zones[1].def.id) assert.is.same('readerfooter_tap', ic._ordered_touch_zones[1].def.id)
assert.is.same('readerhighlight_tap', ic._ordered_touch_zones[2].def.id) assert.is.same('readerhighlight_tap', ic._ordered_touch_zones[2].def.id)
assert.is.same('readermenu_tap', ic._ordered_touch_zones[3].def.id) assert.is.same('readermenu_tap', ic._ordered_touch_zones[3].def.id)
assert.is.same('tap_backward', ic._ordered_touch_zones[4].def.id) assert.is.same('tap_forward', ic._ordered_touch_zones[4].def.id)
assert.is.same('readerhighlight_hold_pan', ic._ordered_touch_zones[5].def.id) assert.is.same('tap_backward', ic._ordered_touch_zones[5].def.id)
assert.is.same('readerfooter_hold', ic._ordered_touch_zones[6].def.id) assert.is.same('readerfooter_hold', ic._ordered_touch_zones[6].def.id)
assert.is.same('readerhighlight_hold', ic._ordered_touch_zones[7].def.id) assert.is.same('readerhighlight_hold', ic._ordered_touch_zones[7].def.id)
assert.is.same('tap_forward', ic._ordered_touch_zones[8].def.id) assert.is.same('readerhighlight_hold_release', ic._ordered_touch_zones[8].def.id)
assert.is.same('readerhighlight_hold_release', ic._ordered_touch_zones[9].def.id) assert.is.same('readerhighlight_hold_pan', ic._ordered_touch_zones[9].def.id)
end) end)
end) end)

@ -19,6 +19,7 @@ describe("Menu widget", function()
callback = cb2 callback = cb2
}, },
}) })
--- @fixme: Currently broken because pairs (c.f., https://github.com/koreader/koreader/pull/6371#issuecomment-657251302)
assert.are.same(re, { assert.are.same(re, {
{ {
text = 'navi', text = 'navi',

Loading…
Cancel
Save